Improve --update-data handler by ikonst · Pull Request #15283 · python/mypy

ikonst · 2023-05-22T15:22:55Z

Fixes #15265.

The new implementation works for two kinds of 'check' tests:

Test cases whose assertions are in [out] and/or [outN] sections, have those sections rewritten with the new results
Test cases whose assertions are inlined as Python comments in [case ...] and [file ...] sections, have the actual results inlined into source code

The two scenarios are mutually exclusive: if [out] is updated, then [case ...] won't be.

Details:

The "fixes" are enqueued and applied in reverse order to prevent line offsets from shifting as it incrementally changes the file.
The update preserves spacing between the source code and comment to reduce diff noise.
Like the previous implementation, we only update "check" data tests.

github-actions · 2023-05-25T00:10:32Z

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

ikonst · 2023-05-25T02:40:57Z

@JelleZijlstra @hauntsaninja Finally (a) merged all those branch-out PRs, and (b) squashed that Windows bug in my test, so it's ready for review

ikonst · 2023-05-30T17:58:57Z

+        # start from end to prevent line offsets from shifting as we update
+        for fix in sorted(self._fixes, reverse=True):
+            lines[fix.lineno - 1 : fix.end_lineno - 1] = fix.lines


A different approach here could have me parse the file, then re-write it while substituting specific sections (i.e. self._fixes would be a mapping of sections to lines), but I think this approach is simple enough.

ikonst · 2023-05-30T19:18:16Z

@hauntsaninja ping

hauntsaninja

Thanks, this is awesome! Tried it out and it worked great on check tests.

hauntsaninja · 2023-06-01T01:11:29Z

Was playing around with this a little more, I think it should probably ignore xfail tests?

ikonst · 2023-06-01T03:14:00Z

Does it end up changing xfail'd tests? Hmpf. Yes, ignoring is the right thing to do (#15337).

cdce8p · 2023-06-06T11:54:54Z

This PR seems to cause a failure in the wheel build. Strangely enough only with x86_64, arm64 is fine. I wasn't able to reproduce the failure unfortunately and CI tests are green as well. Furthermore, the error message seems to be different depending on the platform.
https://github.com/mypyc/mypy_mypyc-wheels/actions/runs/5129904681/jobs/9228043953#step:4:7141 (Linux)
https://github.com/mypyc/mypy_mypyc-wheels/actions/runs/5129904681/jobs/9228044912#step:4:6181 (MacOS)

ikonst · 2023-06-06T14:04:53Z

I think it's a cwd problem, see #15383. The errors across macos and linux appear to be the same.

stdout

  ----------------------------- Captured stdout call -----------------------------
  ============================= test session starts ==============================
  platform linux -- Python 3.11.0rc2, pytest-7.3.1, pluggy-1.0.0
  rootdir: /tmp/tmp.mgLNGp/venv/lib/python3.11
  configfile: pytest.ini
  plugins: forked-1.6.0, xdist-3.3.1, cov-4.1.0
  collected 0 items
  ============================ no tests ran in 0.01s =============================
  `/tmp/tmp.mgLNGp/venv/bin/python -m pytest -n 0 -s --update-data mypy/test/testcheck.py::TypeCheckSuite::check-update-data.test` returned 4: 2 attempts remaining
  ============================= test session starts ==============================
  platform linux -- Python 3.11.0rc2, pytest-7.3.1, pluggy-1.0.0
  rootdir: /tmp/tmp.mgLNGp/venv/lib/python3.11
  configfile: pytest.ini
  plugins: forked-1.6.0, xdist-3.3.1, cov-4.1.0
  collected 0 items
  
  ============================ no tests ran in 0.01s =============================
  `/tmp/tmp.mgLNGp/venv/bin/python -m pytest -n 0 -s --update-data mypy/test/testcheck.py::TypeCheckSuite::check-update-data.test` returned 4: 1 attempts remaining

stderr

  ----------------------------- Captured stderr call -----------------------------
  ERROR: file or directory not found: mypy/test/testcheck.py::TypeCheckSuite::check-update-data.test

This should allow the inner pytest to pass even when the outer pytest is invoked outside of the mypy root, e.g. ```shell cd .. pytest mypy/mypy/test/testupdatedata.py ``` Addresses #15283 (comment).

A5rocks · 2023-07-12T04:12:57Z

I was playing around with this and I think it breaks on multiple errors? I'm getting test case headers overwritten! Might just be a user error though.

ikonst · 2023-07-12T04:33:38Z

@A5rocks Try running pytest -n 0. I think it can happen in parallel invocations (the offsets determined at test time become wrong by update time).

Easiest improvement would be probably to either error out when --update-data is used with parallelism, or make it disable parallelism.

Another option would be to associate changes with identifiers ("update section Y of testcase X to Z"), then parse the file and replay them. Right now the update stage is simple/naive since at that point it's just replacing ranges of lines, not parsing any "INI" structure. (And, of course, we'd still need to hold a lock during the update.)

A5rocks · 2023-07-12T04:38:13Z

That's probably it. Thanks!

Fixes a [gotcha](#15283 (comment)) with `--update-data`. When using `--update-data` with parallelized tests (xdist), the line offsets determined at test time may become wrong by update time, as multiple workers perform their updates to the same file. This makes it so we exit with a usage error when `--update-data` is used with parallelism.

ikonst force-pushed the update_testcase_output branch 2 times, most recently from ea590fb to 9f2425a Compare May 22, 2023 15:27

ikonst marked this pull request as draft May 22, 2023 16:37

ikonst marked this pull request as ready for review May 22, 2023 18:05

ikonst commented May 22, 2023

View reviewed changes

Comment thread mypy/test/data.py Outdated

ikonst commented May 22, 2023

View reviewed changes

Comment thread mypy/test/testcheck.py Outdated

ikonst commented May 22, 2023

View reviewed changes

Comment thread mypy/test/data.py Outdated

This comment has been minimized.

Sign in to view

squash

e76f087

ikonst force-pushed the update_testcase_output branch from 84fc792 to e76f087 Compare May 25, 2023 02:35

factor out into separate file

cf9e0d6

ikonst force-pushed the update_testcase_output branch from e47b5a4 to cf9e0d6 Compare May 25, 2023 05:05

hauntsaninja added the upnext label May 25, 2023

Merge branch 'master' into update_testcase_output

9245583

ikonst commented May 30, 2023

View reviewed changes

hauntsaninja approved these changes May 31, 2023

View reviewed changes

hauntsaninja merged commit 3e03484 into python:master May 31, 2023

ikonst deleted the update_testcase_output branch May 31, 2023 14:34

cdce8p mentioned this pull request Jun 6, 2023

Mypy 1.4 Release Planning #15372

Closed

ikonst mentioned this pull request Jun 6, 2023

testupdatedata: fix cwd #15383

Merged

ikonst mentioned this pull request Jul 12, 2023

tests: --update-data incompatible with xdist #15641

Merged

Conversation

ikonst commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

github-actions Bot commented May 25, 2023

Uh oh!

ikonst commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikonst May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikonst commented May 30, 2023

Uh oh!

hauntsaninja left a comment

Choose a reason for hiding this comment

Uh oh!

hauntsaninja commented Jun 1, 2023

Uh oh!

ikonst commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdce8p commented Jun 6, 2023

Uh oh!

ikonst commented Jun 6, 2023

stdout

stderr

Uh oh!

A5rocks commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikonst commented Jul 12, 2023

Uh oh!

A5rocks commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ikonst commented May 22, 2023 •

edited

Loading

ikonst commented May 25, 2023 •

edited

Loading

ikonst May 30, 2023 •

edited

Loading

ikonst commented Jun 1, 2023 •

edited

Loading

A5rocks commented Jul 12, 2023 •

edited

Loading