Skip to content

Conversation

@elizabethengelman
Copy link
Collaborator

@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
Contributor

@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
Collaborator 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
@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