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(justfile): don't use cargo -p #4184

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Jan 31, 2024

It causes different features/deps being selected, which causes rebuilds of almost whole project, which can be painful.

Instead of cargo test -p pkg it's better to use
cargo nextest run -E 'package(pkg)'.

It causes different features/deps being selected, which causes rebuilds
of almost whole project, which can be painful.

Instead of `cargo test -p pkg` it's better to use
`cargo nextest run -E 'package(pkg)'`.
@dpc dpc requested a review from a team as a code owner January 31, 2024 00:42
@dpc
Copy link
Contributor Author

dpc commented Jan 31, 2024

@m1sterc001guy fyi

@dpc
Copy link
Contributor Author

dpc commented Jan 31, 2024

@m1sterc001guy Idea. Should generating migration aferwards run just .... to test the generated migration (which should be a separate just command for discoverability).

I generated new migration, now I'd like to see if it worked and need to do it manually...

@dpc dpc requested a review from a team as a code owner January 31, 2024 03:22
@dpc
Copy link
Contributor Author

dpc commented Jan 31, 2024

Did a bunch of renaming to unify stuff. I'm open for pushback.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b8ead74) 58.17% compared to head (b752371) 58.17%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4184   +/-   ##
=======================================
  Coverage   58.17%   58.17%           
=======================================
  Files         192      192           
  Lines       43225    43225           
=======================================
+ Hits        25145    25147    +2     
+ Misses      18080    18078    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -114,7 +114,7 @@ mod fedimint_migration_tests {
use fedimint_dummy_common::{DummyCommonInit, DummyInput, DummyOutput};
use fedimint_dummy_server::Dummy;
use fedimint_logging::TracingSetup;
use fedimint_testing::db::{prepare_db_migration_snapshot, validate_migrations, BYTE_32};
use fedimint_testing::db::{snapshot_db_migrations, validate_migrations, BYTE_32};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to use snapshot as a verb to make it more uniform and shorter.

Copy link
Contributor

@m1sterc001guy m1sterc001guy left a comment

Choose a reason for hiding this comment

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

Ack. Good improvement

@m1sterc001guy
Copy link
Contributor

I generated new migration, now I'd like to see if it worked and need to do it manually...

For development it has been convenient for me to generate the snapshot first, just to confirm that the test was written correctly, then run the validation. While developing its just slightly more confusing to run both at the same time since it takes a tad more work to track down where the problem is.

This is just a development workflow though so I can temporarily change the justfile to support that for myself. Your change here is more convenient for everyone else.

@dpc dpc added this pull request to the merge queue Jan 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2024
@dpc dpc added this pull request to the merge queue Jan 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2024
@dpc dpc added this pull request to the merge queue Jan 31, 2024
Merged via the queue into fedimint:master with commit 894ae21 Jan 31, 2024
20 of 21 checks passed
@dpc dpc deleted the 24-01-30-dont-use-cargo-test-p branch January 31, 2024 18:01
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.

None yet

3 participants