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

Chore: rstest improvements #3731

Merged
merged 4 commits into from
Jun 13, 2023
Merged

Chore: rstest improvements #3731

merged 4 commits into from
Jun 13, 2023

Conversation

kantai
Copy link
Member

@kantai kantai commented May 30, 2023

Description

This PR makes a few improvements related to rstest usage in the codebase.

  1. It updates the rstest and rstest-reuse dependencies (these dependencies were generating a future-incompatibilities warning from rustc).
  2. It defines two rstest templates which are reused throughout the tests, rather than defining templates in every module in the testing codebase.
  3. It creates the rstest templates using a macro_rules! macro. This macro includes a test function which includes a match statement. This test function and match statement will trigger a compile time error if an epoch case (in the epochs template) or if an (epoch, clarity_version) pair are unspecified in the template.
  4. It explicitly uses a /tmp/ directory path rather than the rust temp_dir method. This fixes an issue running the tests concurrently in some environments (notably, my development machine!)

Applicable issues

Additional info (benefits, drawbacks, caveats)

This PR only alters testing code.

It's currently based off master, but I can rebase on develop as soon as master -> develop merges.

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #3731 (85d9639) into master (a04f364) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master    #3731     +/-   ##
=========================================
  Coverage    0.17%    0.18%             
=========================================
  Files         302      302             
  Lines      285382   277126   -8256     
=========================================
  Hits          512      512             
+ Misses     284870   276614   -8256     
Impacted Files Coverage Δ
clarity/src/libclarity.rs 0.00% <ø> (ø)
...larity/src/vm/analysis/arithmetic_checker/tests.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/read_only_checker/tests.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/trait_checker/tests.rs 0.00% <0.00%> (ø)
.../src/vm/analysis/type_checker/v2_1/tests/assets.rs 0.00% <0.00%> (ø)
...c/vm/analysis/type_checker/v2_1/tests/contracts.rs 0.00% <0.00%> (ø)
...ity/src/vm/analysis/type_checker/v2_1/tests/mod.rs 0.00% <0.00%> (ø)
clarity/src/vm/tests/assets.rs 0.00% <0.00%> (ø)
clarity/src/vm/tests/contracts.rs 0.00% <0.00%> (ø)
clarity/src/vm/tests/datamaps.rs 0.00% <ø> (ø)
... and 14 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

This is great! Thanks @kantai. Just a couple comments.

mut env_factory: MemoryEnvironmentGenerator,
) {
let mut owned_env = env_factory.get_env(epoch);

let mut placeholder_context = ContractContext::new(
QualifiedContractIdentifier::transient(),
ClarityVersion::Clarity2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and several other places in this file, there are tests where you've added test_clarity_versions but then didn't use version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected in 7c10037. I added a warn(unused) lint definition to the tests module, and fixed all those warnings, then removed the label again, because it interacts pretty unsatisfactorily with the workspace build (it warns on unused items when the testing build flag is set, which generates a ton of invalid warns -- there's probably a way to fix that, but I'll save that for another time).

clarity/src/vm/tests/contracts.rs Show resolved Hide resolved
@saralab saralab requested a review from jbencin June 2, 2023 14:47
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM!

@kantai
Copy link
Member Author

kantai commented Jun 13, 2023

There looks to be two CI issues on this PR (one of which is blocking merging):

  1. create-release is a required status check, but this PR isn't running that action.
  2. It seems like the integration tests workflow only runs when the PR is opened, but not since updating the PR. That's actually not much of an issue for this PR in particular, but it's an issue that should be addressed.

I'll open an issue for this, but just highlighting here as well, @wileyj

@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants