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

Upgrade: Fix test flakiness around upgrade reminders #1381

Merged
merged 3 commits into from
Jul 29, 2019

Conversation

derrickstolee
Copy link
Contributor

Should fix #852.

The fix is to use an environment variable to always show the upgrade message, if present.

While looking at the post-command hook code, I discovered some simple cleanups that are isolated to different commits:

  • Removed unreachable code.
  • Changed a message to refer to gvfs status instead of git status.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
If we fail to communicate with the mount process, then 'gvfs status'
gives us the information we want, not 'git status'.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Use a new environment variable, GVFS_UPGRADE_DETERMINISTIC, to
always print the "upgrade available" message, if it is present.

This should fix test flakiness and issue microsoft#852.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee added type: test-reliability Issues that contribute to test failures affects: tests labels Jul 29, 2019
Copy link
Member

@jamill jamill left a comment

Choose a reason for hiding this comment

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

Thanks!

@derrickstolee
Copy link
Contributor Author

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Copy link
Member

@jrbriggs jrbriggs left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@derrickstolee
Copy link
Contributor Author

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@derrickstolee derrickstolee added this to the M155 milestone Jul 29, 2019
@derrickstolee derrickstolee merged commit e7294c2 into microsoft:master Jul 29, 2019
@wilbaker
Copy link
Member

@derrickstolee with this change the functional tests are no longer testing/validating the upgrade reminder logic in the hooks because GVFS_UPGRADE_DETERMINISTIC is only used in the tests.

Rather than adding the check for GVFS_UPGRADE_DETERMINISTIC would we be better off disabling the test(s) until we have an approach for showing the upgrade message that's deterministic?

@jamill
Copy link
Member

jamill commented Jul 30, 2019

would we be better off disabling the test(s) until we have an approach for showing the upgrade message that's deterministic?

I think this change allows us to test most of the product code path, and provides value. I guess we could get even closer by tweaking the following line:

if ((IsUpgradeMessageDeterministic() || randomValue <= reminderFrequency)

to something like:

int reminderFrequency = IsUpgradeMessageDeterministic() ? 100 : 10;
...
`if ((randomValue <= reminderFrequency)`
...

So this environment variable would only change the frequency (to have it run every time).

@wilbaker
Copy link
Member

I guess we could get even closer by tweaking the following line:
int reminderFrequency = IsUpgradeMessageDeterministic() ? 100 : 10;

@jamill thanks for posting that suggestion, it made me realize I misread the change to the if statement. I incorrectly thought that IsUpgradeMessageDeterministic was skipping the rest of the checks (rather than just skipping the random number portion).

Thanks!

/cc: @derrickstolee

@wilbaker wilbaker mentioned this pull request Aug 6, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: tests type: test-reliability Issues that contribute to test failures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoReminderForLeftOverDownloads test occasionally fails
5 participants