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/ledger tests org update #1343

Merged
merged 12 commits into from
Jun 4, 2024

Conversation

elizabethengelman
Copy link
Contributor

@elizabethengelman elizabethengelman commented May 29, 2024

What

Moves stellar-ledger test-only files into a test_fixtures directory.

Why

Hopefully, this will make it clearer what files are used just for testing, and which ones are source code.

Known limitations

Once this ticket is complete this test-only code will be pulled out into its own crate.

@elizabethengelman elizabethengelman mentioned this pull request May 29, 2024
5 tasks
@elizabethengelman elizabethengelman marked this pull request as ready for review May 29, 2024 14:02
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

You could make the modifications @willemneal mentioned too, but either way this looks like a good improvement 👍🏻.

I don't feel strongly that you need to do this now, but we normally keep the test and test_fixtures modules under the src directory, not at the top level, so that we are still only linking one binary during tests. While Rust does support outside in testing via the tests/ folder at the top level a problem with it is every test file becomes a separate link target, which means the crate has to be relinked over and over, which slows down running tests. For crates this size it may not matter much.

@willemneal
Copy link
Member

@leighmcculloch From my understanding if you have tests/it/main.rs, then it acts as a single target. See https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html#Rules-of-Thumb

So really it's a question about the surface area, should it be external or internal?

@leighmcculloch
Copy link
Member

TIL nice, I didn't know about the module/main.rs approach. 🎉

Also yeah this lib is small so probably not a big deal to worry about test organisation too much. The main goal here I think is reducing confusion about what helper functionality is for tests or not, which this does. 👍🏻

@elizabethengelman
Copy link
Contributor Author

Great discussion, and thanks for sharing that article @willemneal - super helpful!

I also think that if we have tests/module/mod.rs Cargo won't treat the files in that dir as integration tests, and won't be compiled as separate crates, so I think we may also avoid the relinking for each test helper issue... if I'm reading this correctly.

@elizabethengelman elizabethengelman enabled auto-merge (squash) June 4, 2024 19:01
@elizabethengelman elizabethengelman merged commit d7fdbd4 into stellar:main Jun 4, 2024
24 checks passed
@elizabethengelman elizabethengelman deleted the fix/ledger-tests-org-update branch June 4, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants