Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support Junit5 parallel execution #1472 #1519

Merged
merged 2 commits into from
Jan 24, 2023
Merged

support Junit5 parallel execution #1472 #1519

merged 2 commits into from
Jan 24, 2023

Conversation

fladdimir
Copy link
Contributor

resolves #1472

fladdimir
fladdimir commented Jan 11, 2023
View reviewed changes
src/runners/junitRunner/JUnitRunnerResultAnalyzer.ts
this.currentDuration -= start;
}
this.setCurrentState(item, TestResultState.Running, 0);
this.setDurationAtStart(this.getCurrentState(item));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not all changes to this method are strictly necessary, but I felt it becoming a bit lengthy now, so I extracted some details into separate methods)

@fladdimir
Copy link
Contributor Author

(eclipse-jdt/eclipse.jdt.ui#378 will need to be resolved before this feature will work correctly)

@jdneo
Copy link
Member

jdneo commented Jan 12, 2023

@fladdimir Thank you for taking look into this and deal with the upstream issue.

Will it be fine if I review this PR after the upstream changes are merged?

@fladdimir
Copy link
Contributor Author

Will it be fine if I review this PR after the upstream changes are merged?

@jdneo of course thats fine, thanks for the quick reply

@jdneo jdneo self-requested a review January 12, 2023 01:46
@fladdimir
Copy link
Contributor Author

Manual tests (including the locally built upstream fix for the RemoteTestRunner):

before - test2 appears not to be run at all, swallowing its failure:
before.webm

fixed - both test failures are shown, comparison failures and traces are placed correctly:
fixed.webm

@fladdimir
Copy link
Contributor Author

@jdneo the upstream PR has been merged.

Unfortunately I am not familiar with the workflow. I assume the upstream artifact (org.eclipse.jdt.junit.runtime_3.7.x.jar) needs to be published somewhere to become part of the plugins of a eclipse.jdt.ls snapshot build, which could then be included into an upcoming vscode-java extension (preview) release?
(In case that is the way it works, do you have an idea of what could be a timeline for that?)

@jdneo
Copy link
Member

jdneo commented Jan 13, 2023

Cool! I saw the upstream PR has been merged, great job!

The workflow looks like in this way:

  1. The upstream JDT will first release an I-build which includes your changes. I noticed the latest I-build should have already included your changes.
  2. Wait for the vscode-java update the dependency to include the org.eclipse.jdt.junit.runtime which contains your change.
  3. Review your PR, merge it.
  4. After your PR is merged, it will be available very soon in the extension pre-release channel.

I can help update the vscode-java side next week

@fladdimir
Copy link
Contributor Author

  1. The upstream JDT will first release an I-build which includes your changes. I noticed the latest I-build should have already included your changes.
  2. Wait for the vscode-java update the dependency to include the org.eclipse.jdt.junit.runtime which contains your change.
    I can help update the vscode-java side next week

That sounds great, thanks a lot!

@Kropie
Copy link
Contributor

Kropie commented Jan 15, 2023

Awesome work @fladdimir! When I was looking into this I was noticing the issues regarding the interleaving of messages from the eclipse provided unit test runner - great job finding the cause of the issue and working to resolve it!

@jdneo
Copy link
Member

jdneo commented Jan 16, 2023

I raised a PR to update the dependency in VS Code Java: eclipse-jdtls/eclipse.jdt.ls#2403.

since it's about to release a new version this week, that PR should be able to be merged after the release (late this week)

jdneo
jdneo reviewed Jan 18, 2023
View reviewed changes
Copy link
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked the PR today and overall it looks good! 👍

After the latest pre-release of vscode-java is available, I'll play with it manually and merge this PR if everything works fine.

src/runners/junitRunner/JUnitRunnerResultAnalyzer.ts Outdated
// tests may be run concurrently, so each item's current state needs to be remembered
private currentStates: Map<TestItem, CurrentItemState> = new Map();

// failure info for a test is received consecutively:
private currentItem: TestItem | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: how about give it a more specific name, something like: tracingItem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just changed the name 👍🏼

@jdneo
Copy link
Member

jdneo commented Jan 20, 2023

I found that there will be two assertion failure message when running the added sample tests

image

Can we deduplicate then to single one?

@fladdimir
Copy link
Contributor Author

fladdimir commented Jan 20, 2023 •
edited
Loading

I found that there will be two assertion failure message when running the added sample tests
Can we deduplicate then to single one?

While debugging I realized that this duplication of diff-messages also occurs during normal test execution on the current main branch, without being related to parallel test execution.

Apparently its a problem of the general stack trace processing, occuring when a failing equality assertion stack trace contains multiple entries from the test class(?).

(Non-concurrently executed) sample based on the current main branch:

test_duplicated_diff

#1521

There is also a unit-test (part of JUnitAnalyzer.test.ts) to further debug this issue, which reveals that the following is called with a new diff-message each time a matching stack-trace entry is processed:

vscode-java-test/src/runners/baseRunner/RunnerResultAnalyzer.ts

Line 60 in a3848d4

setTestState(this.testContext.testRun, currentItem, TestResultState.Failed, assertionFailure);

I'd like to continue investigating, however, would it be ok to open a separate issue for this?

@fladdimir fladdimir mentioned this pull request Jan 23, 2023
Merged
@jdneo
Copy link
Member

jdneo commented Jan 24, 2023

Sorry for the late reply. I'm on vacation these days :)

Yes, I can confirm that this bug can also be observed in the marketplace version. And it's totally fine to deal with it in a separate issue.

Thank you for the contribution!

@jdneo jdneo merged commit c868001 into microsoft:main Jan 24, 2023
@jdneo jdneo added this to the 0.38.0 milestone Jan 24, 2023
@fladdimir
Copy link
Contributor Author

Sorry for the late reply. I'm on vacation these days :)

Yes, I can confirm that this bug can also be observed in the marketplace version. And it's totally fine to deal with it in a separate issue.

Thank you for the contribution!

no worries at all - thanks for the review and merge!
enjoy your holidays!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdneo jdneo jdneo approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
0.38.1
Development

Successfully merging this pull request may close these issues.

Support JUnit5 parallel execution
3 participants
@fladdimir @jdneo @Kropie