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

deps: disable proc-macro-error default features #134

Merged

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Oct 9, 2023

proc-macro-error has an optional dependency on syn@1 whereas much of the
ecosystem has migrated to syn@2, resulting in dependents of test-case
having to compile syn twice. This avoids that by removing the optional
dependency.

@tamird
Copy link
Contributor Author

tamird commented Oct 9, 2023

@luke-biel

@tamird
Copy link
Contributor Author

tamird commented Oct 10, 2023

Looks like the test failures are unrelated, and are caused by rust 1.73.0.

@tamird tamird force-pushed the proc-macro-error-default-features-false branch 2 times, most recently from 82882f0 to f058688 Compare October 10, 2023 14:59
@tamird
Copy link
Contributor Author

tamird commented Oct 10, 2023

This should be ready for another go-around in CI, if a maintainer would be kind enough to push the button.

@luke-biel
Copy link
Collaborator

Sorry, I wasn't able to properly review the PR yet. If the failures are related to logs, then I'm gonna update the snapshots today and you can merge them in. We do not want the lock against 1.72 in test.

As for contributing to test-case in general - it's very much appreciated :)

@tamird tamird force-pushed the proc-macro-error-default-features-false branch from f058688 to 021f7b3 Compare October 11, 2023 16:40
@tamird
Copy link
Contributor Author

tamird commented Oct 11, 2023

I don't think updating the snapshots is going to cut it because the result is unsatisfying. Here's what I get locally:

diff --git a/tests/snapshots/rust-stable/acceptance__cases_can_be_declared_on_non_test_items.snap b/tests/snapshots/rust-stable/acceptance__cases_can_be_declared_on_non_test_items.snap
index 755cf16..f5cf842 100644
--- a/tests/snapshots/rust-stable/acceptance__cases_can_be_declared_on_non_test_items.snap
+++ b/tests/snapshots/rust-stable/acceptance__cases_can_be_declared_on_non_test_items.snap
@@ -10,4 +10,4 @@ test internal_tested_function3::_1_expects_matching_3_ ... ok
 test internal_tested_function3::_2_expects_inconclusive6 ... ignored
 test internal_tested_function4::_2_expects_panicking_some_can_t_ - should panic ... ok
 test result: FAILED. 4 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.00s
-thread 'internal_tested_function1::_3_expects_6' panicked at 'assertion failed: `(left == right)`
+thread 'internal_tested_function1::_3_expects_6' panicked at src/lib.rs:8:1:
diff --git a/tests/snapshots/rust-stable/acceptance__cases_can_panic.snap b/tests/snapshots/rust-stable/acceptance__cases_can_panic.snap
index 5215572..c50ab6a 100644
--- a/tests/snapshots/rust-stable/acceptance__cases_can_panic.snap
+++ b/tests/snapshots/rust-stable/acceptance__cases_can_panic.snap
@@ -7,6 +7,7 @@ test panicking::_expects_panicking_some_it_has_to_panic_ - should panic ... ok
 test panicking::_expects_panicking_some_this_should_fail_ - should panic ... FAILED
 test panics_without_value::_expects_panicking_none - should panic ... ok
 test pattern_matching_result_fails::simpleenum_var1_expects_matching_simpleenum_var2_ - should panic ... ok
-test result: FAILED. 4 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
-test result_which_panics::_2_2_expects_2_3 - should panic ... ok
-thread 'panicking::_expects_panicking_some_this_should_fail_' panicked at 'It has to panic', src/lib.rs:20:5
+test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
+test result_which_panics::_2_2_expects_2_3 - should panic ... FAILED
+thread 'panicking::_expects_panicking_some_this_should_fail_' panicked at src/lib.rs:20:5:
+thread 'result_which_panics::_2_2_expects_2_3' panicked at src/lib.rs:28:1:
diff --git a/tests/snapshots/rust-stable/acceptance__cases_can_use_regex.snap b/tests/snapshots/rust-stable/acceptance__cases_can_use_regex.snap
index 555c99a..1c3a246 100644
--- a/tests/snapshots/rust-stable/acceptance__cases_can_use_regex.snap
+++ b/tests/snapshots/rust-stable/acceptance__cases_can_use_regex.snap
@@ -11,6 +11,6 @@ test regex_test::_kumkwat_expects_complex_regex_r_ ... FAILED
 test regex_test::_kumkwat_expects_complex_regex_r_abc_ ... FAILED
 test regex_test::_kumkwat_expects_complex_regex_r_kumkwat_ ... ok
 test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
-thread 'regex_test::_abcabc201_expects_complex_regex_r_d_4_' panicked at 'assertion failed: {/n    let re = ::test_case::Regex::new(r#/"//d{4}/"#).expect(/"Regex::new/");/n    re.is_match(_result)/n}', src/lib.rs:5:1
-thread 'regex_test::_kumkwat_expects_complex_regex_r_' panicked at 'Regex::new: Syntax(
-thread 'regex_test::_kumkwat_expects_complex_regex_r_abc_' panicked at 'assertion failed: {/n    let re = ::test_case::Regex::new(r#/"abc/"#).expect(/"Regex::new/");/n    re.is_match(_result)/n}', src/lib.rs:5:1
+thread 'regex_test::_abcabc201_expects_complex_regex_r_d_4_' panicked at src/lib.rs:5:1:
+thread 'regex_test::_kumkwat_expects_complex_regex_r_' panicked at src/lib.rs:5:1:
+thread 'regex_test::_kumkwat_expects_complex_regex_r_abc_' panicked at src/lib.rs:5:1:
diff --git a/tests/snapshots/rust-stable/acceptance__cases_declared_on_non_test_items_can_be_used.snap b/tests/snapshots/rust-stable/acceptance__cases_declared_on_non_test_items_can_be_used.snap
index e593821..5e2f70e 100644
--- a/tests/snapshots/rust-stable/acceptance__cases_declared_on_non_test_items_can_be_used.snap
+++ b/tests/snapshots/rust-stable/acceptance__cases_declared_on_non_test_items_can_be_used.snap
@@ -2,4 +2,4 @@
 source: tests/acceptance_tests.rs
 expression: output
 ---
-thread 'main' panicked at 'Can't', src/lib.rs:33:5
+thread 'main' panicked at src/lib.rs:33:5:
diff --git a/tests/snapshots/rust-stable/acceptance__cases_support_complex_assertions.snap b/tests/snapshots/rust-stable/acceptance__cases_support_complex_assertions.snap
index 43d4632..c56b26a 100644
--- a/tests/snapshots/rust-stable/acceptance__cases_support_complex_assertions.snap
+++ b/tests/snapshots/rust-stable/acceptance__cases_support_complex_assertions.snap
@@ -58,4 +58,4 @@ test not_path::_cargo_toml_parse_unwrap_expects_complex_not_path_dir ... ok
 test not_path::_cargo_yaml_parse_unwrap_expects_complex_not_path_path ... ok
 test not_path::_src_parse_unwrap_expects_complex_not_path_file ... ok
 test result: FAILED. 53 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
-thread 'empty::vec_0_expects_complex_empty' panicked at 'assertion failed: _result.is_empty()', src/lib.rs:115:1
+thread 'empty::vec_0_expects_complex_empty' panicked at src/lib.rs:115:1:
diff --git a/tests/snapshots/rust-stable/acceptance__cases_support_pattern_matching.snap b/tests/snapshots/rust-stable/acceptance__cases_support_pattern_matching.snap
index 41483dd..4cd298a 100644
--- a/tests/snapshots/rust-stable/acceptance__cases_support_pattern_matching.snap
+++ b/tests/snapshots/rust-stable/acceptance__cases_support_pattern_matching.snap
@@ -9,5 +9,5 @@ test extended_pattern_matching_result::simpleenum_var1_expects_matching_ok_e_e_s
 test extended_pattern_matching_result::simpleenum_var2_expects_matching_err_e_e_var2_ ... ok
 test pattern_matching_result::simpleenum_var2_expects_matching_simpleenum_var2_ ... ok
 test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
-thread 'extended_pattern_matching_result::err_should_fail' panicked at 'Expected `Err(e) if e == "var1"` found Err("var2")', src/lib.rs:16:1
-thread 'extended_pattern_matching_result::ok_should_fail' panicked at 'Expected `Ok(e) if e == SimpleEnum :: Var2` found Ok(Var1)', src/lib.rs:16:1
+thread 'extended_pattern_matching_result::err_should_fail' panicked at src/lib.rs:16:1:
+thread 'extended_pattern_matching_result::ok_should_fail' panicked at src/lib.rs:16:1:

in other words, everything useful about the panic message is now absent from the snapshot.

@tamird
Copy link
Contributor Author

tamird commented Oct 12, 2023

@luke-biel any thoughts on this?

@luke-biel
Copy link
Collaborator

@tamird This additional info about why are tests failing is not as necessary as one would think. I meant to rewrite those insta cases for a long time but as you might've noticed I'm short on time.
Anyway - I updated the main branch with these changes, tests now pass on latest stable. I'd be really grateful for a rebase and then we can merge this :)

@tamird
Copy link
Contributor Author

tamird commented Oct 18, 2023

Will do this morning. I'm surprised those updates were acceptable given they no longer contain the representation of the failed assertion.

@luke-biel
Copy link
Collaborator

The point of these tests is not to validate exact context of the failure but rather if the crate behaves as expected in general. I'm more inclined to keep tests simple for now - once they start causing you too much problems it means you we testing wrong.

This will help PR authors determine if CI failures are caused by their
changes or by e.g. Rust releases.
proc-macro-error has an optional dependency on syn@1 whereas much of the
ecosystem has migrated to syn@2, resulting in dependents of test-case
having to compile syn twice. This avoids that by removing the optional
dependency.
@tamird tamird force-pushed the proc-macro-error-default-features-false branch from 021f7b3 to ad54a68 Compare October 18, 2023 15:17
@tamird
Copy link
Contributor Author

tamird commented Oct 18, 2023

Okay. I've now rebased this.

@luke-biel luke-biel merged commit bf3ee27 into frondeus:master Oct 19, 2023
8 checks passed
@tamird tamird deleted the proc-macro-error-default-features-false branch October 19, 2023 13:18
@tamird
Copy link
Contributor Author

tamird commented Oct 19, 2023

Thanks @luke-biel! Would you be willing to cut a patch release?

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

2 participants