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.

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

fix: remove race condition where many suites run in parallel #148

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

tristanzander
Copy link
Contributor

@tristanzander tristanzander commented Oct 23, 2024

Hello! I was attempting to implement the Report Portal jest reporter into our test project when I came across an interesting error. I added some logging statements and this is what I was presented with in my console when I received the failure:

info: Started test "test 1" fx671t90m2m7mmx4
info: Started test "test 2" fx671t90m2m7mmx6
info: Started test "test 3" fx671t90m2m7mmx8
info: finishing step "test 2" fx671t90m2m7mmx8
info: Started test "test 4" fx671t90m2m7mmx9
info: finishing step "test 3" fx671t90m2m7mmx9
info: finishing step "test 4" fx671t90m2m7mmx9
info: finishing step "test 1" fx671t90m2m7mmx9
error: Error: Item with tempId "fx671t90m2m7mmx9" not found

I discovered that, since our test suites run in parallel, the value of JestReportPortal.tempStepId was repeatedly getting modified and re-used when finishing test steps, causing the error. After modifying the JestReportPortal._finishStep method to look into tempStepIds, the issue no longer happens. I had attempted to remove the reliance on JestReportPortal.tempStepId entirely, but it seems like the ReportingApi doesn't have a good way of knowing the name of the current test (at least, one that matches the format of tempStepIds). The only thing I could see that could be used was global.expect.getState().currentTestName, but that's delimited by spaces instead of forward slashes. Theoretically, the reporter could be modified to use space-delimited names and likely fully support concurrent execution.

Please let me know if you need anything else for this PR to be accepted. The issue I was having was a bit tough to test since it relies on tests being finished in a specific order, but I can attempt to do so if necessary. Thanks!

Copy link
Member

@AmsterGet AmsterGet left a comment

Choose a reason for hiding this comment

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

Hello @tristanzander !
I am happy to accept your contribution.
Thanks for your solution and suggestion for further improvements.
I was planning to dive into it at the end of November after vacation.
But right now, I'll release your changes to at least fix the problem with test finishing.

@AmsterGet AmsterGet merged commit 66b71fd into reportportal:develop Nov 1, 2024
2 checks passed
@tristanzander
Copy link
Contributor Author

Hello @tristanzander ! I am happy to accept your contribution. Thanks for your solution and suggestion for further improvements. I was planning to dive into it at the end of November after vacation. But right now, I'll release your changes to at least fix the problem with test finishing.

Of course, thank you for your approval! I'll be sure to contribute any fixes/improvements that my team discovers when using these tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants