Fix make more
#31
Merged
Fix make more
#31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add `make test` target into Makefile that tests both rust and python
skip s3 related rust tests if DOLMA_TESTS_SKIP_AWS=True
|
Also this process made me release that there are no tests for rust targets.
|
|
@chris-ha458 do you mind fixing style errors? thanks! |
make it simpler and clearer what we're trying to do. No need to set to false since we arent going to do anything unless `var==true`
soldni
approved these changes
Aug 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makefile still doesn't work in someways.
This PR plugs the gaps.
To be honest I am not happy with how I am skipping the rust test since they return as successful instead of ignored.
One difficulty is to maintain the tests by default instead of enabling it.
I thought of a solution that is the cleanest :
Prepend all s3 related rust tests with s3 and attach
#[ignore].They would be re-enabled on demand as such :
cargo test s3https://doc.rust-lang.org/book/ch11-02-running-tests.html#filtering-to-run-multiple-tests
However, this might make life more difficult for the current main developers who would have to go through more hoops at the expense of potential outside contributors.
The default settings should be helpful to the default developers.
At this point, my goal is to just make rust tests run and succeed.
IMO we can implement this PR as a stop gap and then move on to a more permanent solution when we can.