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

Reorganize tests #204

Merged
merged 9 commits into from
Jun 1, 2022
Merged

Reorganize tests #204

merged 9 commits into from
Jun 1, 2022

Conversation

Pat-Lafon
Copy link
Contributor

@Pat-Lafon Pat-Lafon commented May 31, 2022

This addresses #113. See the test/README.md for the proposed layout. I've done a best-faith effort to modify the existing test scripts to support this new structure, both in adding turnt *.toml files and in modifying any Makefile make test commands that I could find(brili, brilirs, brilck and brilift).

test/ssa-error/ssa-misslabel had a .err file but no corresponding .bril file which seems to have happened with 52a3981 . I'm not sure if this test is suppose to exist or not so I've added it back.

@Pat-Lafon
Copy link
Contributor Author

Is seems like for test/ssa-error/ssa-misslabel.bril, both brilirs and brili raise an error.

brilirs gives what I think is the right error message error: Line 9, Column 3: Label `here` for phi node not found, but of course brilirs is only checked on it raising an error, not the contents of its message.

brili raises error: undefined variable c which does not match the expected error message and brili is checked on the contents of the message so turnt fails the test.

If the test is invalid it can be removed, otherwise brili should probably be updated to give an appropriate message.

@sampsyo
Copy link
Owner

sampsyo commented Jun 1, 2022

Awesome; thanks!!! A reorganization here was way overdue.

The ssa-misslabel.bril test is actually quite a bugaboo. It's related to the very complicated issue of undefinedness in SSA form discussed in #108. One possible solution to that mess is (was?) letting phi nodes propagate undefinedness… that is, they would be allowed to "un-define" their target variables to emulate the original (non-SSA) program just leaving variables undefined along a given path. This is where brili implements those semantics:

bril/bril-ts/brili.ts

Lines 670 to 671 in e023126

// Last label not handled. Leave uninitialized.
state.env.delete(instr.dest);

Anyway, I suspect the reason I removed that test at some point was just to avoid deciding on what phi instructions should do… so I say let's just keep it removed (and remove its expected output)? The difference between the two interpreters seems A-OK to me given this ambiguity.

@sampsyo
Copy link
Owner

sampsyo commented Jun 1, 2022

About the organization itself: this is best done in a later phase, but I wonder if we shouldn't add a feature to Turnt to avoid the duplication of turnt.toml files. The idea would be to allow Turnt to walk up the directory tree to look for turnt.toml, in the same way git walks up the tree to look for .git. Then we could do something like:

+ test/
| + interp/
  | turnt.toml
  | turnt_brilirs.toml
  | + core/
    | foo.bril
    | bar.bril
  | + fp/
    | baz.bril
    | qux.bril
| + err/
  | turnt.toml
  | turnt_brilirs.toml
  | + core/
    | and_so_on.bril
  | + fp/
    | you_get_the_idea.bril
| + parse/
| + print/

That is, we would have a top level (interp vs. error, for example) that contained a single turnt.toml to cover everything, and below that we could organize stuff by language extension. Those extension-specific sub-directories would not need turnt.tomls of their own because they would share the one from the next level up. Does that make any sense? I've kinda been thinking this would be good to add to Turnt anyway…

@Pat-Lafon
Copy link
Contributor Author

Makes sense to me. I've removed the test.

Not exactly what you were going for but you can fake this a little bit with -c/--config by passing ../turnt.toml instead of the default turnt.toml. It's not as flexible as what you were going for as now some make test commands need to be split up into tests with turnt.toml vs tests with ../turnt.toml. Though I think this is a nice intermediate step until turnt supports walking the directory tree looking for the config file.

This would be a very cool feature for turnt. Thanks for releasing the 1.6 version.

And just to mention, as shown in the commit logs, I've set the Rust github actions to run the tests for bril-rs and brilirs. This wasn't done before because the speculation tests are intentionally failing, thus the motivation for this pr.

@sampsyo
Copy link
Owner

sampsyo commented Jun 1, 2022

Awesome!! This will certainly do for now.

The matrix approach to running multiple kinds of tests in the Rust workflow seems perfectly fine. It does make me wish there were a better way of composing multiple jobs in Actions… some way to build things once and then run multiple tests. The names of the jobs in the GitHub interface are also a little inscrutable, but that is not really a big deal.
Screen Shot 2022-06-01 at 11 40 45 AM

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