Bugs seen in CS thresholds implementation

jrfnl commented 4 years ago

Some things I encountered with the current "CS thresholds" implementation and which are probably bugs:

1. PR build passed with lowering the thresholds, but the trunk merge build failed:

PR build: https://travis-ci.org/github/Yoast/wordpress-seo/jobs/721541137#L1830-L1831 Trunk merge: https://travis-ci.org/github/Yoast/wordpress-seo/jobs/721544642#L1832-L1833

(and no, there were no other commits to trunk between branch off and merge)

2. Trunk merge - errors and warnings threshold are `0` ???

https://travis-ci.org/github/Yoast/wordpress-seo/jobs/721933518#L1793-L1794

3. While the issues were not introduced by this branch, the nr of issues found is higher than the threshold, but the build still passes.

https://travis-ci.org/github/Yoast/wordpress-seo/jobs/722065458

Also: the build currently ignores warnings, so the warnings threshold is basically useless. I suggest turning on warnings.

moorscode commented 4 years ago

https://travis-ci.org/github/Yoast/wordpress-seo/jobs/721933518

Cause of the problem:

You made a reference to a non-existent script @putenv YOASTCS_THRESHOLD_ERRORS=568
You made a reference to a non-existent script @putenv YOASTCS_THRESHOLD_WARNINGS=273

afercia commented 4 years ago

We just noticed that on Travis the threshold check runs but then the build passes even if the threshold has been not honored (at least with some branch names?).

When the errors exceed the threshold, the built is supposed to not pass.

For example, in this PR https://github.com/Yoast/wordpress-seo/pull/16012 there are 966 CS errors against a threshold of 960. However, the build passes.

Looking at the related raw log file: https://api.travis-ci.org/v3/job/725521107/log.txt

The only thing I see (definitely not my area of expertise) is that at some point there's a fatal when attempting to run check_branch_cs:

CODE SNIFFER RESULTS
--------------------
Coding standards errors: 966/960.
Coding standards warnings: 278/264.
Please fix any errors introduced in your code and run composer check-cs-warnings to verify.
Please fix any warnings introduced in your code and run check-cs-thresholds to verify.
> Yoast\WP\SEO\Composer\Actions::check_branch_cs
fatal: ambiguous argument 'feature/semrush': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
No files to compare! Exiting.

Not sure if the branch name containing a slash feature/semrush comes into play.

Yoast / wordpress-seo