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

add Asciicast to regression tests #2376

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

ericphanson
Copy link
Contributor

Based on https://github.com/SciML/SciMLBase.jl/blob/master/.github/workflows/Downstream.yml

Briefly discussed a few years ago here: #1675 (comment)

I've only added my new Asciicast.jl so far, but I imagine we could add all the documenter plugins, if there is interest

@odow
Copy link
Collaborator

odow commented Dec 13, 2023

See https://github.com/JuliaDocs/Documenter.jl/blob/master/.github/workflows/regression-tests.yml

err isa Pkg.Resolve.ResolverError || rethrow()
# If we can't resolve that means this is incompatible by SemVer and this is fine
# It means we marked this as a breaking change, so we don't need to worry about
# Mistakenly introducing a breaking change, as we have intentionally made one
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this just encourages people to make breaking changes 😢

Copy link
Contributor Author

@ericphanson ericphanson Dec 13, 2023

Choose a reason for hiding this comment

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

eh, I think you need something like this if you're going to put the workflow on 20+ repos, like SciML does (where I copied it from). Because it also could be some unrelated dependency has made a breaking change, and the package being tested has updated for it, but one of the regressionees has not, and then you can't resolve, so you can get failing CI unrelated to either package making a breaking release. (Although the comment is not focusing on that case, I suspect it can be common).

Agreed it's probably not necessary here though

@ericphanson ericphanson changed the title add integration tests add Asciicast to regression tests Dec 13, 2023
@ericphanson
Copy link
Contributor Author

Oops, missed that! Ok, I updated this PR to just add Asciicast there

@ericphanson
Copy link
Contributor Author

do we only want workflow-dispatch for regression-tests.yml? That seems OK if we remember to run it, but otherwise we could add it to just run every time

@odow
Copy link
Collaborator

odow commented Dec 14, 2023

I think we should at least attempt to be mindful of CI resources. I'm not overly fond of running the tests of a bunch of unrelated packages every time we fix a typo in Documenter.

And if you're touching internals, Documenter is under no obligation to ensure you don't break. In general, I use the workflow-dispatch in JuMP and MOI quite liberally for anything that seems like it might cause issues. Here's the most recent: jump-dev/MathOptInterface.jl#2359 (comment).

We also talked about adding a TODO item to the release checklist:
https://github.com/JuliaDocs/Documenter.jl/blob/master/docs/src/checklists.md#making-a-release

@mortenpi
Copy link
Member

I'd say let's stick to running this manually. Those tests are informational and not strictly part of the Documenter test suite.

We also talked about adding a TODO item to the release checklist

I thought we already did, apparently not. We definitely should.

@ericphanson
Copy link
Contributor Author

bump

@mortenpi mortenpi merged commit 046b346 into JuliaDocs:master Dec 18, 2023
18 of 21 checks passed
@mortenpi
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants