Use all.sh and its component list in pre-push hook #2639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpg
added
enhancement
mbed TLS team
needs-review
4 tasks
gilles-peskine-arm
requested changes
Oct 31, 2019
gilles-peskine-arm
added
needs-work
and removed
needs-review
It brings no value and distracts us from the actual content. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
The primary purpose is to use it to run all.sh -k -q in the pre-push hook, but this can be useful in any circumstance where you're not interested in the full output from each component and just want a short summary of which components were run (and if any failed). Note that only stdout from components is suppressed, stderr is preserved so that errors are reported. This means components should avoid printing to stderr in normal usage (ie in the absence of errors). Currently all the `check_*` components obey this convention except: - check_generate_test_code: unittest prints progress to stderr - check_test_cases: lots of non-fatal warnings printed to stderr These components will be fixed in follow-up commits. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
The list in the pre-push hook was redundant with the list of `check_*` components in all.sh, and unsurprisingly it was outdated. Missing components were: - check_recursion - check_changelog - check_test_cases - check_python_files - check_generate_test_code Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
force-pushed
the
use-all-sh-checks-for-pre-push
branch
from
25c637f to
129e13c
Compare
|
@gilles-peskine-arm I've force-pushed a version with a new history, as I don't think the initial fiddling in |
mpg
added
needs-review
gilles-peskine-arm
requested changes
Jun 2, 2020
There was a problem hiding this comment.
There are still a few minor problems with output suppression in all.sh.
I timed the old hook at 40s and the new one at 53s. It would be nice to only check what's change, but that would be a lot of work. I don't personally care since I don't use the hook anyway (let the CI take care of testing, that's my philosophy).
Since dd prints everything on stderr, both normal status update and actual errors when they occur, redirecting that to /dev/null is a trade-off that's acceptable in quiet mode (typically used on a developer's machine and the developer will re-run in non-quiet mode if anything fails without sufficient detail in the output), but not that much in non-quiet mode. For example, if our dd invocation fails because the disk in full on a CI machine, we want the error to be reported at the time we invoke dd, and not later when a seemingly unrelated test fails due to an incorrect seedfile. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
While pure sh doesn't have a concept of local variables, we can partially emulate them by unsetting variables before we exit the function, and use the convention of giving them lowercase names to distinguish from global variables. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
|
@gilles-peskine-arm I believe I've addressed all your feedback, except one that you indicated was not a blocker. This is ready for you to review again. |
gilles-peskine-arm
approved these changes
Jun 8, 2020
ronald-cron-arm
approved these changes
Jun 23, 2020
mpg
removed
the
needs-review
This was referenced Jun 23, 2020
gilles-peskine-arm
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently there is some level of duplication between all.sh and the pre-push hook: when a fast check is added to all.sh we have to remember to add it to the pre-push hook as well.
This commit adds five components that were previously not run:
check_recursioncheck_changelogcheck_test_casescheck_python_filescheck_generate_test_codeIt also changes the behaviour in case in-tree cmake was enabled before pushing or otherwise running the pre-push hook: previously the script failed because check_names is not compatible with cmake, now it silently disables cmake (and succeeds unless there is an actual issue).
Status
READY
Requires Backporting
Yes, as it's a test script.
Migrations
NO
Manuel testing
Though the most important case to test is
tests/scripts/all.sh -q -k 'check_*'with and without errors, as that's what's used in the pre-push hook, I believe the full test matrix is 2x2x2:--quiet/--no-quiet--keep-going/--no-keep-goinglibrary/aes.c)I don't think it's really useful at this point to test anything other that the
check_*components with-q.