Fix in testsuite: make one DIR= and make parallel#1574
Conversation
gasche
left a comment
There was a problem hiding this comment.
I believe the patch is correct; good to go if the CI passes.
|
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. |
|
I think this change is wrong.
Indeed, the way things work, it is very important that exec-one executes
only the legacy tests, because the new tests will be run later.
Or am I missing something?
|
|
Ah, indeed, this is the part that was an issue. @objmagic, I think that you need to change the |
|
433309b makes sure that An error occasionally shows up when I test I made a tentative fix 641d519 but I am not very sure it is definitely a correct change |
|
Runhang Li (2018/01/22 19:43 +0000):
@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
Wel mkdir -p is not portable so won't work on Windows.
There is a plan to add mkdir and rmdir to OCaml's stdlib, that's on my
todolist.
On top of that we will be able to implement mkdir -p in OCaml, I think
that's the long-term, clean way.
|
641d519 to
433309b
Compare
|
@shindere I see. I just removed the mkdir change. |
|
@gasche is my change good to go? |
|
I tested the proposed change on the targets For the first three ( |
|
On the other hand, I am a bit worried by the |
5e2a12a to
af2ad1b
Compare
|
I pushed an extra commit, 5130e8a , that fixes the race condition by setting |
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.
…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.
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
|
Thanks @gasche! Let me test |
|
@objmagic: please feel free to squash together the two "Change entry" commits. |
|
@gasche do we need to? I thought all commits will be squashed into one single commit when you press merge button. |
|
Runhang Li (2018/01/30 16:55 -0800):
@gasche do we need to? I thought all commits will be squashed into one
single commit when you press merge button.
Merge can be done in different ways, either squahsing or not squashing
the commits. Some times it's better not to squash them, when they convey
useful information.
|
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
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
Problem:
make one DIR=...andmake parallelno longer work on trunkSolution: patch
testsuite/MakefileTest:
make one DIR=...make one DIR=tests/typing-modulesworks nowmake parallelEnd of output of
make parallel: 940 tests consideredEnd of output of
make all: 940 tests considered