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

Remove Manifests from reference tests #422

Merged
merged 6 commits into from
Sep 1, 2023
Merged

Remove Manifests from reference tests #422

merged 6 commits into from
Sep 1, 2023

Conversation

gdalle
Copy link
Collaborator

@gdalle gdalle commented Aug 31, 2023

Following the discussion in #415, the reference test suite needs weakening.
As soon as PkgTemplates.jl installs dependencies that fall outside the standard library, it is impossible (or very cumbersome) to control that a specific version of every subdependency is selected. As a result, I only include Project.toml files in the reference tests, and no longer Manifest.toml files. In fact, the latter are dropped from the repo altogether thanks to .gitignore.

Ping @hannahilea, this is an easy review

@nickrobinson251
Copy link
Collaborator

do you know why we were ever including these?

@gdalle
Copy link
Collaborator Author

gdalle commented Aug 31, 2023

do you know why we were ever including these?

I wasn't there when the ancient code was written, but I can't think of a compelling reason

@nickrobinson251
Copy link
Collaborator

could you please take a quick look in the history to check there wasn't a good reason

otherwise, i've no objection to this

@gdalle
Copy link
Collaborator Author

gdalle commented Aug 31, 2023

For one thing there is #143: it's useful to check that the parent package is indeed dev-ed in the docs environment, and this cannot be seen from the Project alone

@nickrobinson251
Copy link
Collaborator

nice find! i think that's maybe an important test -- can we keep that test somehow? e.g. keep the docs/Manifest.toml (perhaps with a more limited test that just checks for the presence of the package we expect to be dev'ed)?

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #422 (3632a6a) into master (2c1b526) will decrease coverage by 38.79%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #422       +/-   ##
===========================================
- Coverage   93.59%   54.81%   -38.79%     
===========================================
  Files          23       23               
  Lines         687      686        -1     
===========================================
- Hits          643      376      -267     
- Misses         44      310      +266     

see 17 files with indirect coverage changes

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

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 1, 2023

I went for the easy solution of only discarding test/Manifest.toml for now, so that this and #415 can be merged. Testing the dev-ing of the package into docs/Manifest.toml is a lot of boilerplate, and might have to be repeated for benchmark/Manifest.toml or the like, so I'd rather do incremental progress.

Copy link
Collaborator

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

thanks!

@nickrobinson251 nickrobinson251 merged commit 16c0a73 into JuliaCI:master Sep 1, 2023
@hannahilea
Copy link
Contributor

Nice!

Might be worth filing issue so that adding back coverage of #422 (comment) isn't lost

@nickrobinson251
Copy link
Collaborator

don't we still have that coverage, because we kept the docs/Manifest.toml? Or are we filtering them out too? As far as i understand, we should be able to guarantee the docs/Manifest.toml versions (for a fixed Documenter version), no?

@hannahilea
Copy link
Contributor

You're right---we're only filtering out joinpath("test", "Manifest.toml")), not all "Manifest.toml". Good!

@cossio
Copy link
Contributor

cossio commented Sep 2, 2023

Any reason to keep WackyOptions/Manifest.toml around? Causes a CI failure at #426, which is otherwise green.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 2, 2023

I think it is related to #423

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.

4 participants