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 404 during project reset test #740

Merged
merged 22 commits into from
Apr 26, 2024
Merged

Fix 404 during project reset test #740

merged 22 commits into from
Apr 26, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Apr 19, 2024

Fixes #728.

This fixes the race conditions I identified in #728 (comment) by narrowing the window during which they can happen from seconds to microseconds. It's still theoretically possible that the hgweb refresh scan might happen to trigger right between the project being deleted and the new project being moved into place, but the chances of that happening are miniscule, and we should see it only once in a blue moon.

Now that we've removed the race condition in ResetProject, we no longer
need to wait for the hgweb refresh interval in the middle of the
SendReceiveAfterProjectReset tests. (We still do need to wait for it at
the start of the test, after project creation).
Copy link

github-actions bot commented Apr 19, 2024

C# Unit Tests

36 tests  +6   36 ✅ +6   5s ⏱️ -1s
 8 suites +1    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 7819539. ± Comparison against base commit e1b2f7d.

♻️ This comment has been updated with latest results.

If the zip file uploaded during the reset project step had no .hg
directory anywhere inside it, we want to delete it entirely before
presenting the error message to the user. Otherwise we might leave quite
large amounts of useless data lying around in the repos volume.
@rmunn rmunn self-assigned this Apr 19, 2024
@rmunn rmunn requested a review from myieye April 19, 2024 06:53
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I think minimizing the time when a repo is missing sounds like a great idea!
I hope it really is the root cause.

There are a few things that I think could be a bit tidier.

backend/LexBoxApi/Services/HgService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Outdated Show resolved Hide resolved
@myieye
Copy link
Contributor

myieye commented Apr 23, 2024

So, I commited a test that sadly didn't manage to reproduce the 404 😢, but it still fairly heavily tests this feature, so it seems reasonable to keep it around.

And then I resolved a merge conflict I introduced 😉

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

Also, I still think that we should use a job to reset project meta-data when a user finishes resetting with a zip. That's part of #728 and will mean that Entry-Count also gets updated.

@rmunn
Copy link
Contributor Author

rmunn commented Apr 24, 2024

Also, I still think that we should use a job to reset project meta-data when a user finishes resetting with a zip. That's part of #728 and will mean that Entry-Count also gets updated.

I added that in commit b2f6ef0, but without using a job. That way: 1) it will fail quickly if we're going to get 404s on the newly-reset project, so we'll find out sooner if we've solved the problem with hgweb or not. And 2) unlike Send/Receive, the FinishReset process is not especially time-sensitive, so updating the lex entry count immediately doesn't matter nearly as much as it does in a Send/Receive scenario.

However, if I'm proved wrong about those points, I'll push another commit moving this to a job.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

Nice work, though I'm not super sure this actually makes that much of a difference. Especially for the moving an empty repo case.

backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/Testing/ApiTests/ResetProjectRaceConditions.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Show resolved Hide resolved
backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Show resolved Hide resolved
backend/LexBoxApi/Services/HgService.cs Show resolved Hide resolved
@rmunn
Copy link
Contributor Author

rmunn commented Apr 25, 2024

Okay, I tried to reply to multiple comments at once in the Files view of the PR, so that you'd only get a single notification email. But this is how GitHub formats it when you do that? Ugly. From now on I'll just reply in the Conversation view, where the formatting is much better. You'll get a notification email for each reply — sorry about that — but it'll be much easier to follow the conversation.

This saves seeral seconds of test setup time.
This allows the project to build when MercurialVersion is set to 6.
@rmunn
Copy link
Contributor Author

rmunn commented Apr 25, 2024

@myieye - Let me know what you think of the change I made to your IntegrationFixture code, where instead of downloading a .zip file and creating an hg commit inside it, we just store the .zip file in our Git repo (and it already has the .hg folder inside it so we don't need to create a new commit).

The one behavior change that this makes is that with your code, the template repo would have a different initial commit hash every time you run the tests (but it would have the same initial commit hash for every test run during a single dotnet test session). Whereas by storing a .zip file (that already contains an initial commit) in our Git repo and re-using it each time, we will have the same initial commit hash on every test run.

As far as I can tell, none of our tests actually relied on the initial commit hash being unpredictable and/or changing from run to run, so I think this change has no downsides. But if it does become a problem sometimes, we can always copy the template repo, delete .hg from the copy, and re-run the hg init; hg add; hg commit sequence in that copy so that its initial commit hash will be different. (This would allow us, for example, to test the scenario of a user doing a Send/Receive to an unrelated repo, which Chorus checks for and forbids). Note that we would have needed to do that anyway with the existing setup, because the initial commit hash, though varying from run to run, would be the same across all tests in a single run.

@rmunn
Copy link
Contributor Author

rmunn commented Apr 25, 2024

Note: now that #750 has been merged, we'll need to double-check the parts of this PR that touch project metadata and make sure they're still correct (now that #750 makes both FLEx and WeSay projects have a lex entry count).

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

looks good, I like the addition of the zip so we're not downloading it during the test.

I added some tests for the fixture as that's getting more complicated.

backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/Testing/Fixtures/IntegrationFixture.cs Outdated Show resolved Hide resolved
backend/Testing/Fixtures/IntegrationFixtureTests.cs Outdated Show resolved Hide resolved
The user typeahead had two debounces, one in the input and one in the
typeahead store. The debounce in the input was causing Playwright tests
to fail because they were filling in the input and expecting the form to
be valid immediately, but it wasn't valid until the debounce triggered.
The debounce on the input now serves no purpose; removing it.
Many times the Playwright code gets to the email page before the email
has actually shown up. So we need to make it wait for up to 10 seconds,
refreshing the email list periodically (because mailDev, at least,
doesn't auto-refresh: you have to click the refresh button).
@rmunn
Copy link
Contributor Author

rmunn commented Apr 26, 2024

Commits 9d9413f and b4a3150 are the solution to #759, which I figured would be better included here since without the FinishReset changes, they might not work as well as they should.

BTW, all 30 Playwright tests are now green for me, on both Chromium and Firefox, with b4a3150 included.

…-404

Fixes merge conflicts, and I've verified that the GetLexEntryCount
changes for WeSay are still correct.
Copy link

github-actions bot commented Apr 26, 2024

UI unit Tests

11 tests  ±0   11 ✅ ±0   0s ⏱️ ±0s
 3 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 7819539. ± Comparison against base commit e1b2f7d.

♻️ This comment has been updated with latest results.

@rmunn rmunn linked an issue Apr 26, 2024 that may be closed by this pull request
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I did a bit more Playwright stuff and added some mocking so that @hahn-kev's new Fixture tests aren't implicitly integration tests any more.

I'd like to get the test fixes and improvements into the next release. So, I'm gonna merge this.

@hahn-kev I wouldn't be surprised if you'd have done things slightly differently...feel free to refactor of course.

@myieye myieye merged commit ec3b763 into develop Apr 26, 2024
11 checks passed
@rmunn
Copy link
Contributor Author

rmunn commented Apr 29, 2024

I added that in commit b2f6ef0, but without using a job. That way: 1) it will fail quickly if we're going to get 404s on the newly-reset project, so we'll find out sooner if we've solved the problem with hgweb or not. And 2) unlike Send/Receive, the FinishReset process is not especially time-sensitive, so updating the lex entry count immediately doesn't matter nearly as much as it does in a Send/Receive scenario.

However, if I'm proved wrong about those points, I'll push another commit moving this to a job.

Looks to me like I will need to make it a job: running the update immediately after FinishReset is, for some reason, returning 0 (because Mercurial still sees the repo as being empty and thinks tip is commit 0000000000). It looks like I might need to push the LexEntryCount update into a job after all.

@myieye
Copy link
Contributor

myieye commented Apr 29, 2024

@rmunn It seems likely to me that this would be fixed by a good fix for #765. What do you think?

@myieye
Copy link
Contributor

myieye commented Apr 29, 2024

I can confirm that this PR unfortunately didn't fix the 404s.
Here's a new occurence.
But, I think #765 sounds promising.

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.

Make emailWorkflow tests more robust GetLastCommitTimeFromHg after project reset sometimes => 404
3 participants