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 test cases that delete a file during incremental checking #3461

Merged
merged 5 commits into from
May 30, 2017

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented May 26, 2017

No description provided.

@@ -154,6 +154,8 @@ def run_case_once(self, testcase: DataDrivenTestCase, incremental_step: int = 0)
# change. We manually set the mtime to circumvent this.
new_time = os.stat(target).st_mtime + 1
os.utime(target, times=(new_time, new_time))
for path in testcase.deleted_paths.get(incremental_step, set()):
os.remove(path)
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll need a try/except around this similar to other places (see tear_down() in same file). That should prevent the AppVeyor flakes (in general on Windows remove() sometimes fails due to e.g. virus checkers having the file open).

@@ -147,15 +147,17 @@ def run_case_once(self, testcase: DataDrivenTestCase, incremental_step: int = 0)
if file.endswith('.' + str(incremental_step)):
full = os.path.join(dn, file)
target = full[:-2]
shutil.copy(full, target)
# Use retries to work around potential flakiness on Windows (AppVeyor).
retry_on_error(lambda: shutil.copy(full, target))
Copy link
Member

Choose a reason for hiding this comment

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

I was surprised to see that you need to retry the copy too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A test case might first delete a file and the reintroduce it here, and I suspect that this could then require a retry. Not 100% sure though, but it seems better to be defensive since test flakes are very annoying.


# In some systems, mtime has a resolution of 1 second which can cause
# annoying-to-debug issues when a file has the same size after a
# change. We manually set the mtime to circumvent this.
new_time = os.stat(target).st_mtime + 1
os.utime(target, times=(new_time, new_time))
for path in testcase.deleted_paths.get(incremental_step, set()):
os.remove(path)
# Use retries to work around potential flakiness on Windows (AppVeyor).
retry_on_error(lambda: os.remove(path))
Copy link
Member

Choose a reason for hiding this comment

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

The test failures seem to be due to the lack of a return type context. Maybe the retry_on_error() function should just take a Callable[[], None]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the return type to Any. The type check might be worth filing as a mypy bug, since it looks like mypy should be able to infer the call -- I'll think about it.

@gvanrossum
Copy link
Member

I guess there's another try/except needed (but this one doesn't look like it should retry?)

@gvanrossum gvanrossum self-assigned this May 30, 2017
@JukkaL
Copy link
Collaborator Author

JukkaL commented May 30, 2017

This failure could be different. We don't hit that code path on Linux at all during the failing test case, as far as I can see.

@JukkaL JukkaL merged commit 977eeec into master May 30, 2017
carljm added a commit to carljm/mypy that referenced this pull request May 30, 2017
* master: (23 commits)
  Make return type of open() more precise (python#3477)
  Add test cases that delete a file during incremental checking (python#3461)
  Parse each format-string component separately (python#3390)
  Don't warn about returning Any if it is a proper subtype of the return type (python#3473)
  Add __setattr__ support (python#3451)
  Remove bundled lib-typing (python#3337)
  Move version of extensions to post-release (python#3348)
  Fix None slice bounds with strict-optional (python#3445)
  Allow NewType subclassing NewType. (python#3465)
  Add console scripts (python#3074)
  Fix 'variance' label.
  Change label for variance section to just 'variance' (python#3429)
  Better error message for invalid package names passed to mypy (python#3447)
  Fix last character cut in html-report if file does not end with newline (python#3466)
  Print pytest output as it happens (python#3463)
  Add mypy roadmap (python#3460)
  Add flag to avoid interpreting arguments with a default of None as Optional (python#3248)
  Add type checking plugin support for functions (python#3299)
  Mismatch of inferred type and return type note (python#3428)
  Sync typeshed (python#3449)
  ...
@gvanrossum gvanrossum deleted the test-delete-file-2 branch June 19, 2017 14:51
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.

2 participants