Skip to content

Add pause between computing lastModified date and executing task #27073

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

Merged
8 commits merged into from
Nov 3, 2020

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Oct 20, 2020

Addresses #25623

Instead of checking the last modified date on the file to check it was modified, this PR uses an FS watcher to check if the file has been changed, making it more defensive towards any issues that come up with the last write time on the file.

Edit: This uses a wait now.

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 20, 2020
@captainsafia captainsafia marked this pull request as ready for review October 20, 2020 19:36
@captainsafia captainsafia requested review from javiercn and a team October 21, 2020 16:05
@captainsafia
Copy link
Member Author

We discussed a few options for addressing this flakiness during the engineering sync including:

  • Sticking with using the FileSystemWatcher, which apparently doesn't work as reliably if there's multiple modifications are made on the FS.
  • Add a delay in between when lastModified is generated and when the TaskInstance is executed
  • Remove the evaluation of the last modified date altogether considering we already validate if the bundle content changes in this test

Pinging @dotnet/aspnet-blazor-eng to close on the final choice between the two. I'm leaning towards #3 if it's reasonable.

@captainsafia captainsafia changed the title Use FileSystemWatcher to check if file was modified Add pause between computing lastModified date and executing task Nov 2, 2020
@ghost
Copy link

ghost commented Nov 3, 2020

Hello @captainsafia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 24b34d7 into master Nov 3, 2020
@ghost ghost deleted the safia/fix-test branch November 3, 2020 21:55
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
…net/aspnetcore#27073)

* Use FileSystemWatcher to check if file was modified

* Update ConcatenateFilesTest.cs

* Update ConcatenateFilesTest.cs

* Update ConcatenateFilesTest.cs

* Update ConcatenateFilesTest.cs

* Update ConcatenateFilesTest.cs

* Update ConcatenateFilesTest.cs

* Use Task.Delay

Commit migrated from dotnet/aspnetcore@24b34d720366
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants