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

Point out validate.sh more prominently in CONTRIBUTING.md #10330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

9999years
Copy link
Collaborator

The CONTRIBUTING.md makes it sound really hard to run tests locally, but actually validate.sh works fine!

validate.sh still needs work (see the list in #10320) for example, but it's a great starting point.

@9999years 9999years added the merge me Tell Mergify Bot to merge label Sep 6, 2024
@ffaf1
Copy link
Collaborator

ffaf1 commented Sep 6, 2024

Pointing that validate.sh runs everything locally is fine, but it is far from a tl;dr, isn't it?

Adding a sentence or a paragraph feels better. Something like “You can also run all tests locally using validate.sh”.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

to Francesco's point: I think it's good enough. But if someone feels strong about it, they should feel free to "request changes".

@geekosaur
Copy link
Collaborator

It might be worth pointing out that, especially with the njobs change, running validate locally can have a pretty significant impact on a personal machine. (Admittedly, not as much so as building ghc….)

@ffaf1
Copy link
Collaborator

ffaf1 commented Sep 9, 2024

Yes, my suggestion is to:

  • add after “Running tests locally.”: “./validate.sh` runs all the test suites.”
  • add “There are two ways to run tests, using GitHub Actions and running them locally” as first paragraph of “Running tests” heading.

This way we should be clear and immediate enough for all use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants