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

Make file expectations always check all expected files at once #1076

Merged
merged 9 commits into from
Aug 31, 2023

Conversation

CraftSpider
Copy link
Contributor

@CraftSpider CraftSpider commented Aug 25, 2023

This keeps bothering me while debugging bibtex failures, and just flipping the file order there would work, but it would be a band-aid, not a real solution. So here's a real solution - make all expectations test once, and panic once at the end.

(Also removed genio from bibtex tests, as it just confuses the output and all the useful logs go to both stdout and the file, IME)

@CraftSpider
Copy link
Contributor Author

Quick thought: Would it be possible to make CI prioritize, say, fmt, clippy, and one of the common/fastest platform tests? Since those seem to be the most likely to fail, might help with turnaround of realizing things are broken on a PR.

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #1076 (a26d912) into master (e625488) will increase coverage by 0.24%.
Report is 27 commits behind head on master.
The diff coverage is 62.36%.

❗ Current head a26d912 differs from pull request most recent head 545fa2e. Consider uploading reports for the commit 545fa2e to get more accurate results

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
+ Coverage   45.96%   46.21%   +0.24%     
==========================================
  Files         171      171              
  Lines       64180    64843     +663     
==========================================
+ Hits        29499    29964     +465     
- Misses      34681    34879     +198     
Files Changed Coverage Δ
crates/docmodel/src/workspace.rs 70.00% <ø> (+2.85%) ⬆️
crates/engine_bibtex/bibtex/bibtex.c 51.86% <ø> (+1.12%) ⬆️
crates/engine_bibtex/src/c_api/xbuf.rs 51.42% <ø> (-1.35%) ⬇️
crates/engine_bibtex/src/c_api/log.rs 44.91% <39.08%> (-4.22%) ⬇️
crates/engine_bibtex/src/lib.rs 64.70% <47.61%> (-2.46%) ⬇️
crates/engine_bibtex/src/c_api/pool.rs 84.38% <55.17%> (-8.01%) ⬇️
crates/engine_bibtex/src/c_api/exec.rs 53.34% <56.86%> (+25.24%) ⬆️
crates/engine_bibtex/src/c_api/cite.rs 78.53% <58.33%> (-5.52%) ⬇️
crates/engine_bibtex/src/c_api/scan.rs 71.18% <70.07%> (-24.69%) ⬇️
crates/engine_bibtex/src/c_api/bibs.rs 83.33% <76.92%> (-7.81%) ⬇️
... and 7 more

... and 14 files with indirect coverage changes

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

@pkgw
Copy link
Collaborator

pkgw commented Aug 26, 2023

@CraftSpider The main tests should all run in parallel, so in principle there shouldn't be a wait ... but maybe if we hit the limit for simultaneous jobs, and the dispatcher files them in lexical order, there would be an issue? You could try editing dist/azure-build-and-test.yml to put things like Clippy at the beginning of the jobs: section, and see if that changes things? I think that if you do it in this PR, the tests here will be run with that setup.

@CraftSpider
Copy link
Contributor Author

Sounds like it's worth a try - I consistently see only some running, I kind of figured it was some max concurrency limit.

@pkgw
Copy link
Collaborator

pkgw commented Aug 31, 2023

Did the reordering of the Azure Pipelines file seem to help?

@CraftSpider
Copy link
Contributor Author

Yep, it appears putty clippy and rustfmt at the top gave them more 'priority' in the queue - they tend to now be the first to finish, which is nice.
(I could run clippy locally, but I enable a bunch of extra lints through a dev-env global config for personal projects, and haven't yet gotten around to making tectonic ignore them. Also I just forget to run it before pushing a lot :P)

@pkgw pkgw merged commit 6330269 into tectonic-typesetting:master Aug 31, 2023
26 checks passed
@CraftSpider
Copy link
Contributor Author

Want to note, since it isn't in any of the comments on this PR: This removed the cross-mips build, due to the recent Rust decision to downgrade these platforms to T3, and consequent removal of host-tools builds for them causing CI to fail. If they are ever re-upgraded to T2 (could happen if someone steps up as a maintainer) the build can be easily re-added.

@pkgw
Copy link
Collaborator

pkgw commented Aug 31, 2023

I did notice that, thanks. This means that we lose our big-endian test build, which is why I added MIPS in the first place. That's too bad but it doesn't seem to exactly be a major use case these days, and hopefully the existing code won't bit-rot on us.

@CraftSpider CraftSpider deleted the improve-tests branch December 1, 2023 02:04
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.

2 participants