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

Add preserveTestName CLI flag to remove partition and browser #963

Merged

Conversation

tasha-urbancic
Copy link
Contributor

@tasha-urbancic tasha-urbancic commented Jul 12, 2022

Closes: #129

I came across the same issue as #129 . Specifically, when I increased our parallelization and added --random we lost CI insights as the output changes. To increase consistency in the output I have added an optional flag --preserve-test-name that removes the dynamic segments from the output. This is a first attempt at solving the issue, so perhaps it isn't the best way.

Because this is is my first time contributing, I am not sure if I am missing something important here. I would love feedback. Thanks in advance!

@tasha-urbancic tasha-urbancic force-pushed the feature/preserve-test-name branch 2 times, most recently from 3fc3a76 to 5cd11ba Compare July 19, 2022 22:31
@tasha-urbancic tasha-urbancic marked this pull request as ready for review July 19, 2022 22:44
@tniezurawski
Copy link
Collaborator

tniezurawski commented Sep 28, 2022

This is a real issue. We have insights enabled on CircleCI and because of splitting and parallelizing tests, we can't benefit from it to the fullest. Here's an example of flaky tests. We know they fail more often but the name of tests are not consistent so the counting doesn't work as it should:

image

That of course affects all analyses, like the slowest tests. We see clear duplicates there, the difference comes only from the prepended info about partition and browser... which to be honest, doesn't give us much 🤷‍♂️

image

How can I help make it happen? Who's approving and releasing this project? @rwjblue? @Turbo87? @SergeAstapov?

@SergeAstapov
Copy link
Contributor

@tniezurawski can't help with pushing this through. but changes looks good to me! thanks for adding tests, this really helps.

README.md Outdated
ok 2 Chrome 66.0 - Exam Partition 1 - browser Id 2 - another test
ok 3 Chrome 66.0 - Exam Partition 1 - browser Id 3 - some the other test
```
However, if you change the amount of parallelization, or randomize accross partitions, the output will change for the same test, which may be an issue if you are tracking test insights over time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small typo:

Suggested change
However, if you change the amount of parallelization, or randomize accross partitions, the output will change for the same test, which may be an issue if you are tracking test insights over time.
However, if you change the amount of parallelization, or randomize across partitions, the output will change for the same test, which may be an issue if you are tracking test insights over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I have fixed the typo.

@tniezurawski
Copy link
Collaborator

tniezurawski commented Sep 28, 2022

Thanks @SergeAstapov 🙌 and thanks @tasha-urbancic for the change 😍

@tasha-urbancic Maybe consider adding "Closes: #129" to the description so it should close the issue automatically after this change is merged.

Copy link
Collaborator

@tniezurawski tniezurawski left a comment

Choose a reason for hiding this comment

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

Just tested on my CI and see the proper result in xunit report (running on 4 containers x 3 browsers each):

<testcase classname="Chrome 106.0" name="Acceptance | 404 page: should redirect user to 404 page when using wrong URL" time="0.261"/>

🎉

@tniezurawski
Copy link
Collaborator

By looking at the releases page, I think @Turbo87 and @rwjblue are maintainers of this package. What can we do to make this change happen?

@ef4
Copy link
Contributor

ef4 commented Nov 16, 2022

Hi @tasha-urbancic and @tniezurawski, would either or both of you be willing to be added as maintainers to this package?

@tniezurawski
Copy link
Collaborator

@ef4 Yes, I'd like to 👍

@tniezurawski tniezurawski merged commit c48bc2f into ember-cli:master Jan 19, 2023
@tasha-urbancic
Copy link
Contributor Author

@ef4 same! 👍

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

Successfully merging this pull request may close these issues.

separate partition number from test name
5 participants