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

Move lit tests so no symlink is needed #4615

Merged
merged 14 commits into from
Oct 18, 2023

Conversation

keyboardDrummer
Copy link
Member

Changes

Move Littish tests from Test to Source/IntegrationTests/Test. This way there's no symlink needed which causes Rider to behave annoyingly

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@keyboardDrummer keyboardDrummer changed the title Move lit tests Move lit tests so no symlink is needed Oct 5, 2023
@keyboardDrummer keyboardDrummer enabled auto-merge (squash) October 5, 2023 12:51
@robin-aws
Copy link
Member

Great idea!

Having trouble browsing/commenting on specific files since GitHub only loads the first 3000 files and my browser keeps hanging. :) I'll just make comments here instead:

  1. Could you reformat the commits so that the big Test relocation is in its own dedicated commit? That way I can exclude it when viewing the changes.
  2. I intended the XUnitExtensions project to be fairly generic and for some of it to apply to any tests with files as input data, not just littish ones. I suspect some of our language server tests could be streamlined a bit to use it as well and make it easier to add more test files for the same test harness. That's why the default location for a test's files is $"TestFiles/{class name}/{test method name}". I strongly prefer to not hardcode that to Test instead. Instead we have two better options: we can either relocate the Test files under IntegrationTests/TestFiles/LitTests/LitTest directly, or you can override the Directory property of FileDataAttribute on the LitTest method to be "Test" instead. I prefer the former since "Test" as a subdirectory is pretty meaningless without the history behind it, but either would be fine.
  3. Given how long the Test directory has existed in this project, I would keep it there with just a small README that says "These files have been relocated under ...", just to give contributors a hand if they hit nasty merge conflicts or get confused.

@keyboardDrummer
Copy link
Member Author

@robin-aws could you review the current diff? I think it addresses your comments. I've excluded the actual move for now since it's easier to review and then the PR doesn't accrue conflicts. Once you approve it I'll do the actual move, reapprove, and then merge.

@robin-aws
Copy link
Member

Looks great and sounds like a plan, thanks! :)

@keyboardDrummer keyboardDrummer merged commit 601f8a4 into dafny-lang:master Oct 18, 2023
18 checks passed
@keyboardDrummer keyboardDrummer deleted the moveLitTests branch October 18, 2023 16:25
robin-aws pushed a commit that referenced this pull request Oct 18, 2023
Move Littish tests from `Test` to `Source/IntegrationTests/Test`. This
way there's no symlink needed which causes Rider to behave annoyingly

<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
RustanLeino added a commit that referenced this pull request Oct 19, 2023
Recently (#4615), the Dafny test
suite moved from `Test/` to
`Source/IntegrationTests/TestFiles/LitTests/LitTest/`. This PR makes a
corresponding change in the `respositoryRoot` definition in
`lit.site.cfg`. This allows `lit` to be run in the new location.

<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
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