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

Syntest reports #167

Merged
merged 4 commits into from
May 28, 2018
Merged

Syntest reports #167

merged 4 commits into from
May 28, 2018

Conversation

trishume
Copy link
Owner

This adds syntax tests to CI and makes it easier to check for regressions locally using them!

I was about to spend some time debugging syntax test regressions in #166 but then I realized I still hadn't gotten around to making regressions be caught automatically and be easy to digest.

I switched syntest to use getopts since syncat was already using it, and then added a --summary option which only prints failures and failure counts. I then added a make syntest command to diff against known issues and fail if there's a difference, and a make update-known-failures to easily update the counts on submodule bump PRs and things, or when we fix some.

I don't print the total number of assertions in summaries so that adding new succeeding tests doesn't cause CI failures.

cc @sharkdp

@trishume trishume requested review from robinst and keith-hall May 27, 2018 15:12
@sharkdp
Copy link
Contributor

sharkdp commented May 27, 2018

@trishume I just tested this, it works great. I was looking for something like this and ended up writing a small script myself for the output in #166.

Just one small thing: you should probably sort the output before building the diff because directory-walking is not deterministic:

--- testdata/known_syntest_failures.txt	2018-05-27 17:17:45.865872515 +0200
+++ -	2018-05-27 17:19:31.194805415 +0200
@@ -1,6 +1,6 @@
 loading syntax definitions from testdata/Packages
-FAILED testdata/Packages/ASP/syntax_test_asp.asp: 162
 FAILED testdata/Packages/C#/tests/syntax_test_Strings.cs: 38
 FAILED testdata/Packages/LaTeX/syntax_test_latex.tex: 1
 FAILED testdata/Packages/Makefile/syntax_test_makefile.mak: 7
+FAILED testdata/Packages/ASP/syntax_test_asp.asp: 162
 exiting with code 1

@trishume
Copy link
Owner Author

@sharkdp As soon as I submitted the CI run I guessed that might be a problem. Walkdir iterates in sorted order only on macOS apparently.

I fixed it by making syntest output in sorted order instead, which properly preserves messages at the start and end and also leads to consistent syntest output for other scripts.

Copy link
Collaborator

@keith-hall keith-hall left a comment

Choose a reason for hiding this comment

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

looks great, nice work - I was also thinking something like this would be useful

@sharkdp
Copy link
Contributor

sharkdp commented May 27, 2018

I can rebase my branch in #166 on master after this is merged - to test if CI fails.

Copy link
Collaborator

@robinst robinst left a comment

Choose a reason for hiding this comment

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

Great idea :)! Changes look good.

@trishume trishume merged commit 42f3da4 into master May 28, 2018
@trishume trishume deleted the syntest-reports branch May 28, 2018 12:35
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.

4 participants