Skip to content

Remove duplicate Logos import from doctests in tests/lib.rs#523

Merged
jeertmans merged 2 commits intomaciejhirsz:masterfrom
RoyPrinsGH:fix_double_import_in_doctests
Jan 2, 2026
Merged

Remove duplicate Logos import from doctests in tests/lib.rs#523
jeertmans merged 2 commits intomaciejhirsz:masterfrom
RoyPrinsGH:fix_double_import_in_doctests

Conversation

@RoyPrinsGH
Copy link
Contributor

@RoyPrinsGH RoyPrinsGH commented Dec 10, 2025

While reading through the book's guidelines on contributing, it suggested that I run cargo test --workspace. This command failed however, due to these doctests importing Logos both from logos and logos_derive.

I removed those double imports and now it's all good :)

Before
image
After
image

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 10, 2025

CodSpeed Performance Report

Merging #523 will not alter performance

Comparing RoyPrinsGH:fix_double_import_in_doctests (ce57ff3) with master (f4a22d9)

Summary

✅ 6 untouched

@jeertmans
Copy link
Collaborator

Hi @RoyPrinsGH, thanks for catching this! We should probably include doctests within the GitHub workflows. Would you mind editing .github/workflows/rustlib.yml such that we also cover those tests?

@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.67%. Comparing base (f4a22d9) to head (ce57ff3).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   64.78%   64.67%   -0.11%     
==========================================
  Files          29       29              
  Lines        1820     1820              
==========================================
- Hits         1179     1177       -2     
- Misses        641      643       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeertmans
Copy link
Collaborator

Actually, the tests are now failing, see logs. This is probably because you ran tests with the wrong configuration... I don't know what's best.

@RoyPrinsGH
Copy link
Contributor Author

RoyPrinsGH commented Dec 10, 2025

It seems I was too eager -- the tests now failed due to not finding Logos in the... logos package...

I'll look into it

@RoyPrinsGH
Copy link
Contributor Author

RoyPrinsGH commented Dec 10, 2025

@jeertmans You guys do run doc-tests (check any other logs, scroll all the way down).

What I think is happening is that running cargo test --workspace pulls in default features of logos (here the culprit is the Logos derive) while running cargo test in the tests folder doesn't, since that Cargo.toml in tests only uses logos with the std feature, not the export_derive feature

I decided to just pull the derive from the existing logos_derive import

@tisonkun
Copy link
Contributor

I can try to help with this issue. Looks like we'd leverage trybuild for first-class compile tests rather than depending on doctests + compile_fail derivative.

@jeertmans
Copy link
Collaborator

Makes sense, thanks for your contribution @RoyPrinsGH!

@jeertmans jeertmans merged commit 20d7db9 into maciejhirsz:master Jan 2, 2026
20 of 21 checks passed
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.

3 participants