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

Merged
merged 5 commits into from
Sep 28, 2024

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.

@9999years
Copy link
Collaborator Author

I've addressed comments & made the headings uniform. (Use the rich diff to see changes.)

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

very good

@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Sep 26, 2024
@Mikolaj Mikolaj force-pushed the contributing-test-instructions branch from 6c2b6c3 to 1d57a33 Compare September 28, 2024 22:41
@mergify mergify bot merged commit 65230be into haskell:master Sep 28, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants