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

test: Fixing a weird problem happening when running the autojoin tests #32614

Closed
wants to merge 7 commits into from

Conversation

Gustrb
Copy link
Contributor

@Gustrb Gustrb commented Jun 14, 2024

Proposed changes (including videos or screenshots)

For some reason they would sometimes pass, and sometimes fail with weird error messages.
There were some weird after/afterEach in this test which I decided to remove to check what was happening and it looks like that did the trick :)

Issue(s)

Steps to test or reproduce

Further comments

for some reason they would sometimes pass, and sometimes fail with weird error messages
There were some weird after/afterEach in this test which I decided to remove to check what was happening
and it looks like that did the trick
Copy link
Contributor

dionisio-bot bot commented Jun 14, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Jun 14, 2024

⚠️ No Changeset found

Latest commit: 3c71e8e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.72%. Comparing base (4f72d62) to head (3c71e8e).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32614      +/-   ##
===========================================
- Coverage    56.39%   55.72%   -0.68%     
===========================================
  Files         2479     2416      -63     
  Lines        54580    53470    -1110     
  Branches     11268    11002     -266     
===========================================
- Hits         30780    29794     -986     
+ Misses       21125    21052      -73     
+ Partials      2675     2624      -51     
Flag Coverage Δ
e2e 54.79% <ø> (-1.29%) ⬇️
unit 72.00% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@Gustrb Gustrb added this to the 6.10 milestone Jun 14, 2024
@Gustrb Gustrb marked this pull request as ready for review June 14, 2024 22:38
@Gustrb Gustrb requested a review from a team as a code owner June 14, 2024 22:38
@Gustrb Gustrb requested a review from ggazzo June 14, 2024 22:39
@Gustrb Gustrb added the stat: QA assured Means it has been tested and approved by a company insider label Jun 17, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 17, 2024
Comment on lines 1628 to 1631
afterEach(() =>
Promise.all([deleteTeam(credentials, testTeam.name), deleteRoom({ roomId: createdRoom.body.channel._id, type: 'c' })]),
);

Copy link
Member

Choose a reason for hiding this comment

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

this should continue in "after" if the test fails it will not remove the rooms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yea, my bad

@Gustrb Gustrb requested a review from ggazzo June 17, 2024 18:22
@ggazzo ggazzo removed the stat: QA assured Means it has been tested and approved by a company insider label Jun 19, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Jun 19, 2024
@Gustrb
Copy link
Contributor Author

Gustrb commented Jun 19, 2024

The root cause was that sometimes users were being created with the same name, since the name is created through a Random Number Generator, #32634 is the correct solutions.
I assumed this PR passing all the tries was an indication that it solved the flakiness, but it appears to have been sheer (bad) luck.

@Gustrb Gustrb closed this Jun 19, 2024
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.

None yet

2 participants