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

testsuite: convert typing-unboxed-types to expect-style tests #2128

Merged
merged 1 commit into from Oct 31, 2018

Conversation

Projects
None yet
3 participants
@gasche
Copy link
Member

commented Oct 31, 2018

This will be very useful for an upcoming PR implementing a different
unboxing check, which change the result for some of the tests. Without
the present change, the diff in test results was unreadable, which
is problematic for development and for code review.

@gasche gasche force-pushed the gasche:unboxed-types-testsuite branch from c4300f5 to 2e59668 Oct 31, 2018

testsuite: convert typing-unboxed-types to expect-style tests
This will be very useful for an upcoming PR implementing a different
unboxing check, which change the result for some of the tests. Without
the present change, the diff in test results was unreadable, which
is problematic for development and for code review.
@trefis

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

A nice property of the previous version of the test (i.e. before your diff) was that we were sure that the same thing was tested with and without flat float arrays. Now, nothing's preventing the files from diverging.

Is there some work that could be done on expect_test to have both with and without flat float arrays results in the same file / expect block (similarly to what is done for -principal)?

@diml

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

It's possible, however it means that the user will have to run the test with both configurations manually.

It works as follow for -principal: every node has two expectations: one without -principal and one with. During one run, only one of the expectation is updated, corresponding to the current setting. When both expectations are equal, they are displayed as a single expectation. Because -principal is a runtime flag, the test harness runs the test twice in a row: once without -principal and once with. After the first step, many nodes might have two different expectations, however after the second run most of them will be collapsed again to a single expectation.

Because flat float arrays is not a runtime flag, the test harness cannot do the two runs automatically. So it means that the user would see the intermediate state where only one of the expectation has been updated, which might be a bit confusing.

I suggest to do the following: allow to disable tests depending on the compiler settings. When the outcome of a test depends on the flat float arrays setting, it should be duplicated into two tests: one that is enabled iff flat float arrays if off and one that is enabled iff flat float arrays is on. Then the CI should test both configurations to make sure all tests are up to date.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

Note: if "duplicating into two tests" means also duplicating the tested code, then it is what I have done manually here. Duplicating within a single file is nicer because the two copies remain close together, but still not so nice. On the other, duplicating expectations only

foo;;
[%%expect ~when:Config.flat-float-array {|...|}]
[%%expect ~when:(not Config.flat-float-array) {|...|}]

would work nicely.
(Would that require parsing and interpreting the ~when argument at ppx-processing time? Wild stuff!)

In the specific case of this PR, frankly I think that "testing the same thing on both side" is overrated: in the -no-flat-float-array case, the check is trivial, so the only thing we are testing is the dynamic behavior (which is not actually tested in most cases). Since I don't want to wait for hypothetical expect-test features, I think the change as proposed is a good compromise. I could also remove the test-noflat, or keep only the ones that have some interesting dynamic behavior.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

P.S.: @trefis, do you approve of this reasoning?

@diml

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

I hadn't considered manually duplicating the expectation this way, but yes it should work well.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

In the specific case of this PR, frankly I think that "testing the same thing on both side" is overrated: in the -no-flat-float-array case, the check is trivial, so the only thing we are testing is the dynamic behavior (which is not actually tested in most cases). Since I don't want to wait for hypothetical expect-test features, I think the change as proposed is a good compromise. I could also remove the test-noflat, or keep only the ones that have some interesting dynamic behavior.

Ack.
I think it's indeed fine to take the PR as is until someone finds the time to update expect_test.

@trefis

trefis approved these changes Oct 31, 2018

@gasche gasche merged commit fd5c692 into ocaml:trunk Oct 31, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

Thanks!

Should we track the conditional-expectations idea / feature request somewhere?

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.