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 in testsuite: `make one DIR=` and `make parallel` #1574

Merged
merged 4 commits into from Jan 31, 2018

Conversation

Projects
None yet
3 participants
@objmagic
Copy link
Contributor

objmagic commented Jan 18, 2018

Problem: make one DIR=... and make parallel no longer work on trunk

Solution: patch testsuite/Makefile

Test:

  • make one DIR=...
    make one DIR=tests/typing-modules works now
  • make parallel
    End of output of make parallel: 940 tests considered
    End of output of make all: 940 tests considered
@gasche
Copy link
Member

gasche left a comment

I believe the patch is correct; good to go if the CI passes.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 18, 2018

Note: there was a previous attempt to fix this issue ( #1530 ), and it ran into trouble with the CI, so it is important to check that the CI passes indeed.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Jan 18, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 18, 2018

Ah, indeed, this is the part that was an issue.

@objmagic, I think that you need to change the legacy* rules to not run exec-one when [ -f $(DIR)/ocamltests ] holds.

@gasche gasche dismissed their stale review Jan 18, 2018

Changes still required

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Jan 22, 2018

@gasche @shindere

433309b makes sure that legacy does not run ocamltest-based tests

@shindere

An error occasionally shows up when I test make clean && make parallel on my 24-core workstation:

Running tests from 'tests/ast-invariants' ...
mkdir: cannot create directory ‘/aaa/bbb/projects/ocaml/testsuite/_ocamltest’: File exists
Sysem command mkdir "/aaa/bbb/projects/ocaml/testsuite/_ocamltest" failed with status 1

I made a tentative fix 641d519 but I am not very sure it is definitely a correct change

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Jan 22, 2018

@objmagic objmagic force-pushed the objmagic:objmagic/fix-make-one-parallel branch from 641d519 to 433309b Jan 22, 2018

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Jan 22, 2018

@shindere I see. I just removed the mkdir change.

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Jan 27, 2018

@gasche is my change good to go?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 27, 2018

I tested the proposed change on the targets all, legacy, new, parallel, one.

For the first three (all, legacy, new), the behavior is the same as in trunk -- according to report at least. For parallel, trunk is broken (only "legacy" tests are run) and the branch works correctly (all tests are run). For one, trunk is broken (new tests are not run) and the branch works correctly.

@gasche

gasche approved these changes Jan 27, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 27, 2018

On the other hand, I am a bit worried by the mkdir race condition worried above. I wonder if it is possible to invoke the tool so that no shared testsuite/_ocamltest directory is created at all, and _ocamltest directories are created from the test directories?

@gasche gasche force-pushed the objmagic:objmagic/fix-make-one-parallel branch from 5e2a12a to af2ad1b Jan 28, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 28, 2018

I pushed an extra commit, 5130e8a , that fixes the race condition by setting OCAMLTESTDIR to each local test directory in the exec-one target (the new target is unchanged and keeps a global _ocamltest directory for all tests).

objmagic and others added some commits Jan 18, 2018

Fix in testsuite: `make one` and `make parallel`
The `parallel` and `one DIR=...` targets currently do not work as
inteded for tests using the new 'ocamltest' framework: `one ...` has
no effect, and `parallel` only runs on old-style tests and ignores new
tests. This commit fixes this discrepancy in behavior.
testsuite/Makefile: fix _ocamltest race with per-test OCAMLTESTDIR se…
…tting

When using 'make parallel', one could sometimes observe an error due
to a race condition in creating the global _ocamltest repository used
by all new-style (ocamltest) tests. This commit removes the race by
making exec-one use local _ocamltest directories, one for each test
directory (the parallelism of the 'parallel' target is at the
granularity of each test directory, not each test file, so this
is safe)

On the contrary, the global 'new' and 'new-without-report' targets
still run all test in a global BASEDIR/_ocamltest directory. This
choice was done to minimize difference in behavior for users of these
targets, but it could be revisited in the future.

gasche added a commit to gasche/ocaml that referenced this pull request Jan 28, 2018

ocamltest: implement a -promote option
When -promote is set, running the test automatically overwrites each
reference output file with the actual output of the test. This option,
which mirrors the "make promote" target of old-style testsuite tests,
is very useful when a minor change in compiler/toplevel output affects
a lot of reference files in innocuous ways.

This is the only option of ocamltest that overwrites file in the
source directory, so it should be used with care -- under strict
version-control supervision.

The 'make promote' target is not yet supported for new-style testsuite
tests, as this feature depends on the not-yet-merged GPR#1574

  ocaml#1574

For a previous discussions of this feature, see GPR#1519

  ocaml#1519
@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Jan 28, 2018

Thanks @gasche! Let me test make parallel out on my machine tomorrow

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 30, 2018

@objmagic: please feel free to squash together the two "Change entry" commits.

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Jan 31, 2018

make parallel works on my workstation now - no race condition in ocamltest.

@gasche do we need to? I thought all commits will be squashed into one single commit when you press merge button.

@gasche gasche merged commit a5d9a70 into ocaml:trunk Jan 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Jan 31, 2018

gasche added a commit to gasche/ocaml that referenced this pull request Feb 2, 2018

ocamltest: implement a -promote option
When -promote is set, running the test automatically overwrites each
reference output file with the actual output of the test. This option,
which mirrors the "make promote" target of old-style testsuite tests,
is very useful when a minor change in compiler/toplevel output affects
a lot of reference files in innocuous ways.

This is the only option of ocamltest that overwrites file in the
source directory, so it should be used with care -- under strict
version-control supervision.

The 'make promote' target is not yet supported for new-style testsuite
tests, as this feature depends on the not-yet-merged GPR#1574

  ocaml#1574

For a previous discussions of this feature, see GPR#1519

  ocaml#1519

gasche added a commit to gasche/ocaml that referenced this pull request Feb 7, 2018

ocamltest: implement a -promote option
When -promote is set, running the test automatically overwrites each
reference output file with the actual output of the test. This option,
which mirrors the "make promote" target of old-style testsuite tests,
is very useful when a minor change in compiler/toplevel output affects
a lot of reference files in innocuous ways.

This is the only option of ocamltest that overwrites file in the
source directory, so it should be used with care -- under strict
version-control supervision.

The 'make promote' target is not yet supported for new-style testsuite
tests, as this feature depends on the not-yet-merged GPR#1574

  ocaml#1574

For a previous discussions of this feature, see GPR#1519

  ocaml#1519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.