Add CI test to ensure that README.md files are present and correct #386

coriolinus · 2017-11-05T13:43:48Z

Checking readmes takes up non-trivial maintainer time, and is
prone to error. This script adds a check to the CI which ensures
that PRs are only accepted if their README.md files are
present and correct.

The intent and method come from the discussion which begins here.

It begins by isolating the exercises already impacted by the PR,
so that upstream changes to unrelated exercises don't cause undue
difficulty. It then simply ensures that the README.md within
the PR is identical to the one generated by configlet generate.

Outputs lists of exercises with missing or incorrect READMEs,
so people for whom CI fails due to this test not passing should
have an easy time understanding what went wrong and how to fix it.

Checking readmes takes up non-trivial maintainer time, and is prone to error. This script adds a check to the CI which ensures that PRs are only accepted if their README.md files are present and correct. It begins by isolating the exercises already impacted by the PR, so that upstream changes to unrelated exercises don't cause undue difficulty. It then simply ensures that the README.md within the PR is identical to the one generated by `configlet generate`. Outputs lists of exercises with missing or incorrect READMEs, so people for whom CI fails due to this test not passing should have an easy time understanding what went wrong and how to fix it.

petertseng

I advise that before merging this PR you should test that the build does fail when expected to. I realise that I have contradicted this advice by only doing #377 and #376 after the fact, but I had reason to believe local tests were sufficient. Here is my reason why I believe a test now would be superior to a local test:

I suspect that problem-specifications needs to be present.
If this suspicion holds true, problem-specifications needs to be made to exist. The choices are to clone it in the added script, or add a before_install section that clones it.

petertseng · 2017-11-05T18:21:27Z

_test/ensure-readmes-are-updated.sh

+      bin/configlet generate . --only "$exercise"
+      generated_readme_checksum=$(md5sum $readme_path | cut -d' ' -f1)
+
+      if [ $existing_readme_checksum != $generated_readme_checksum ]; then


optional: I imagine you can check the exit code of diff rather than comparing the checksums against one another, if you find that easier.

We don't actually have both files in place at the same time; the configlet command to regenerate it overwrites the existing README.md, if any. For this reason, I thought it easier to just keep the md5sum in memory and compare that.

petertseng · 2017-11-05T18:31:27Z

Now is the time to mention this.

If ever we come across the situation:

The README in the Rust track should differ from the current problem-specifications description, e.g. as it needed to in Add two-bucket test #375 since the problem-specifications description unnecessarily constrained us
The change is unsuitable for inclusion in problem-specifications, perhaps because it is language-specific
The change cannot be expressed by using .meta/hints.md to append content to the problem-specifications description, because it is iether a deletion of a line, an insertion into the middle of the document, or an edit of an existing line (this third can be expressed as a composition of the former two)
We don't want to maintain a separate .meta/description.md because the change is small and we don't want to force ourselves to manually follow every single problem-specifications change from then on

We may wish for an alternative way to express we want a different README. I was thinking a solution based on a .patch file. Solution is not fully formed yet because it has not become necessary. Kind of hoping it never does, but you never know.

Checking readmes takes up non-trivial maintainer time, and is prone to error. This script adds a check to the CI which ensures that PRs are only accepted if their README.md files are present and correct. It begins by isolating the exercises already impacted by the PR, so that upstream changes to unrelated exercises don't cause undue difficulty. It then simply ensures that the README.md within the PR is identical to the one generated by `configlet generate`. Outputs lists of exercises with missing or incorrect READMEs, so people for whom CI fails due to this test not passing should have an easy time understanding what went wrong and how to fix it.

coriolinus · 2017-11-05T19:44:46Z

I advise that before merging this PR you should test that the build does fail when expected to

I'm not sure how to test this on Travis without actually merging it. I've performed local tests by adding a do-not-merge branch with missing or incorrect READMEs; here's a transcript of redoing that:

coriolinus@robocomp:~/projects/exercism.io/xrust$ git checkout -b do-not-merge-test-ensure-readmes-are-updated
Switched to a new branch 'do-not-merge-test-ensure-readmes-are-updated'
do-not-merge-test-ensure-readmes-are-updated
coriolinus@robocomp:~/projects/exercism.io/xrust$ git rm exercises/decimal/README.md
rm 'exercises/decimal/README.md'
do-not-merge-test-ensure-readmes-are-updated│#1
coriolinus@robocomp:~/projects/exercism.io/xrust$ git rm exercises/all-your-base/README.md
rm 'exercises/all-your-base/README.md'
do-not-merge-test-ensure-readmes-are-updated│#2
coriolinus@robocomp:~/projects/exercism.io/xrust$ echo "bogus line" >> exercises/crypto-square/README.md
do-not-merge-test-ensure-readmes-are-updated│~1#2
coriolinus@robocomp:~/projects/exercism.io/xrust$ echo "another bogus line" >> exercises/hello-world//README.md
do-not-merge-test-ensure-readmes-are-updated│~2#2
coriolinus@robocomp:~/projects/exercism.io/xrust$ git add exercises/
do-not-merge-test-ensure-readmes-are-updated│#4
coriolinus@robocomp:~/projects/exercism.io/xrust$ git commit -m "state needs to be committed for the script to work"
[do-not-merge-test-ensure-readmes-are-updated 6d6fc27] state needs to be committed for the script to work
 4 files changed, 2 insertions(+), 128 deletions(-)
 delete mode 100644 exercises/all-your-base/README.md
 delete mode 100644 exercises/decimal/README.md
do-not-merge-test-ensure-readmes-are-updated
coriolinus@robocomp:~/projects/exercism.io/xrust$ _test/ensure-readmes-are-updated.sh
Checking readme for all-your-base
Checking readme for crypto-square
Checking readme for decimal
Checking readme for hello-world
Exercises missing README.md:
  all-your-base
  decimal
Exercises with out-of-date README.md:
  crypto-square
  hello-world
do-not-merge-test-ensure-readmes-are-updated│~2
coriolinus@robocomp:~/projects/exercism.io/xrust$ echo $?
1

coriolinus · 2017-11-05T19:52:35Z

If ever we come across the situation: ...

Agreed that there should probably exist some way to handle that. A .patch file may work, but I think it's out of scope for this PR; that is more a problem for configlet.

coriolinus · 2017-11-05T19:56:31Z

Here's an example of the existing script correctly accepting a valid README:

coriolinus@robocomp:~/projects/exercism.io/xrust$ touch exercises/decimal/tests/more_tests.rs
ensure-readmes-are-updated│+1
coriolinus@robocomp:~/projects/exercism.io/xrust$ git checkout -b do-not-merge-test-ensure-readmes-are-updated-success
Switched to a new branch 'do-not-merge-test-ensure-readmes-are-updated-success'
do-not-merge-test-ensure-readmes-are-updated-success│+1
coriolinus@robocomp:~/projects/exercism.io/xrust$ git add exercises/ && git commit -m "ensure that exercises with modifications but correct READMES pass"
[do-not-merge-test-ensure-readmes-are-updated-success 348f527] ensure that exercises with modifications but correct READMES pass
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 exercises/decimal/tests/more_tests.rs
do-not-merge-test-ensure-readmes-are-updated-success
coriolinus@robocomp:~/projects/exercism.io/xrust$ _test/ensure-readmes-are-updated.sh
Checking readme for decimal
do-not-merge-test-ensure-readmes-are-updated-success
coriolinus@robocomp:~/projects/exercism.io/xrust$ echo $?
0

coriolinus · 2017-11-05T20:04:07Z

I suspect that problem-specifications needs to be present.

I'm fairly confident that it does not. Reasoning: I have successfully used configlet to generate READMEs without having a local clone of problem-specifications anywhere on my machine. It makes more sense for it to pull directly from a canonical Github location for each exercise anyway, IMO, because it can be sure that Github will be up-to-date; the same cannot be said of a local clone.

Still, I'm not actually 100% confident of this. Perhaps someone familiar with configlet internals could chime in with an answer?

petertseng · 2017-11-05T20:41:26Z

I'm not sure how to test this on Travis without actually merging it. I've performed local tests by adding a do-not-merge branch with missing or incorrect READMEs

That's good, then you can either put the commit(s) on this PR, ensure that Travis fails to build then, then revert them, or put them either on a separate branch on your fork (configure Travis to build your fork) or make a PR for it.

I have successfully used configlet to generate READMEs without having a local clone of problem-specifications anywhere on my machine.

whoaaaaaaaaaaa

I have no idea how that can work at all, since according to https://github.com/exercism/configlet/blob/master/cmd/generate.go#L55-L58 if it does not exist, it is an error.

Unfortunately I cannot verify that myself since I am not able to run configlet myself, so I will have to take the word of you + Travis for that.

This commit is designed to cause Travis to fail, if exercism#386 is merged and implemented correctly. It should check the following exercises, modified by this PR: all-your-base crypto-square decimal hello-world rectangles two-bucket The following two exercises should be reported as missing README.md: all-your-base decimal The followign two exercises should be reported as having out of date READMEs.md: crypto-square hello-world The following two exercises should be checked, but pass without further output: rectangles two-bucket Note that if Travis runs on these without including the `_test/ensure-readmes-are-updated.sh` test, it will likely pass without comment.

coriolinus · 2017-11-05T22:29:04Z

I have successfully used configlet to generate READMEs without having a local clone of problem-specifications anywhere on my machine.

whoaaaaaaaaaaa

Looks like I was wrong: I certainly thought I'd done this before ever becoming a maintainer, for the second exercise I submitted, but the command did fail in Travis. I'm going to add a section which clones the problem-specifications repo within the script; it seems the most direct path to solving this problem.

…ready exist This enables us to point configlet at a known location where it should exist, which in turn should make Travis happier with this.

…ercism_rust into ensure-readmes-are-updated Conflicts: _test/ensure-readmes-are-updated.sh

This commit is designed to cause Travis to fail, if exercism#386 is merged and implemented correctly. It should check the following exercises, modified by this PR: all-your-base crypto-square decimal hello-world rectangles two-bucket The following two exercises should be reported as missing README.md: all-your-base decimal The followign two exercises should be reported as having out of date READMEs.md: crypto-square hello-world The following two exercises should be checked, but pass without further output: rectangles two-bucket Note that if Travis runs on these without including the `_test/ensure-readmes-are-updated.sh` test, it will likely pass without comment.

I'm going to blame Git for this one, though it's probably user error

coriolinus · 2017-11-06T23:06:55Z

#387 now fails the CI in the expected manner, so I've closed it. It now serves as documentation that this PR works as expected.

petertseng

Are you OK with the $ being printed? Note the $ at end of each line.

Exercises missing README.md:$
  all-your-base$
  decimal
Exercises with out-of-date README.md:$
  crypto-square$
  hello-world

coriolinus · 2017-11-06T23:15:12Z

That's caused by differences between bash and sh. It's not ideal, but it has no functional effect, so I'm not bothered by it. The solution would be to have Travis run this script via bash instead of sh, which would be inconsistent with the rest of the test scripts.

I'll wait a day or two in case there are more comments, then merge this.

I don't feel like checking this manually. Lifted straight from the Rust track. exercism/rust#386

coriolinus requested review from kytrinyx and petertseng November 5, 2017 13:44

petertseng reviewed Nov 5, 2017

View reviewed changes

coriolinus mentioned this pull request Nov 5, 2017

DO NOT MERGE: Examples for #386 #387

Closed

coriolinus added 2 commits November 5, 2017 23:33

Clone the problem specifications into a subdirectory if it doesn't al…

7aae551

…ready exist This enables us to point configlet at a known location where it should exist, which in turn should make Travis happier with this.

Merge branch 'ensure-readmes-are-updated' of github.com:coriolinus/ex…

be9df7c

…ercism_rust into ensure-readmes-are-updated Conflicts: _test/ensure-readmes-are-updated.sh

coriolinus added 2 commits November 5, 2017 23:40

Redo updates to script.

cf66a28

I'm going to blame Git for this one, though it's probably user error

Use https instead of ssh git path for problem-specifications

7923464

petertseng approved these changes Nov 6, 2017

View reviewed changes

coriolinus merged commit 822eb17 into exercism:master Nov 7, 2017

coriolinus deleted the ensure-readmes-are-updated branch November 7, 2017 23:47

petertseng mentioned this pull request Dec 4, 2017

travis: Ensure READMEs are updated exercism/haskell#629

Merged

petertseng added a commit to exercism/haskell that referenced this pull request Dec 4, 2017

travis: Ensure READMEs are updated (#629)

76ff1ce

I don't feel like checking this manually. Lifted straight from the Rust track. exercism/rust#386

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CI test to ensure that README.md files are present and correct #386

Add CI test to ensure that README.md files are present and correct #386

coriolinus commented Nov 5, 2017

petertseng left a comment

petertseng Nov 5, 2017

coriolinus Nov 5, 2017

petertseng commented Nov 5, 2017 •

edited

Loading

coriolinus commented Nov 5, 2017 •

edited

Loading

coriolinus commented Nov 5, 2017

coriolinus commented Nov 5, 2017

coriolinus commented Nov 5, 2017

petertseng commented Nov 5, 2017

coriolinus commented Nov 5, 2017 •

edited

Loading

coriolinus commented Nov 6, 2017

petertseng left a comment

coriolinus commented Nov 6, 2017

Add CI test to ensure that README.md files are present and correct #386

Add CI test to ensure that README.md files are present and correct #386

Conversation

coriolinus commented Nov 5, 2017

petertseng left a comment

Choose a reason for hiding this comment

petertseng Nov 5, 2017

Choose a reason for hiding this comment

coriolinus Nov 5, 2017

Choose a reason for hiding this comment

petertseng commented Nov 5, 2017 • edited Loading

coriolinus commented Nov 5, 2017 • edited Loading

coriolinus commented Nov 5, 2017

coriolinus commented Nov 5, 2017

coriolinus commented Nov 5, 2017

petertseng commented Nov 5, 2017

coriolinus commented Nov 5, 2017 • edited Loading

coriolinus commented Nov 6, 2017

petertseng left a comment

Choose a reason for hiding this comment

coriolinus commented Nov 6, 2017

petertseng commented Nov 5, 2017 •

edited

Loading

coriolinus commented Nov 5, 2017 •

edited

Loading

coriolinus commented Nov 5, 2017 •

edited

Loading