-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Replace Paver quality and js_test commands [Paver Removal 2/3] #35159
Replace Paver quality and js_test commands [Paver Removal 2/3] #35159
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@feanil @kdmccormick |
@salman2013 as you remove Paver commands, please remove the tests that correspond to those commands. |
Thanks for the PR @salman2013, but this only goes part of the way. The goal of #35159 is not only to get rid of the Keep in mind that there are several things that pavelib code did that are now completely unnecessary, for example:
I recommend starting with one simple check, such as |
@kdmccormick Could you please look into my comment #34845 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just "Requesting Changes" so this doesn't show up in my queue. The direction is looking great-- just re-request my review whenever you're ready Salman.
@kdmccormick This PR is ready for another pass. Please take a look thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work so far. Really happy to see these running without Paver.
@kdmccormick I believe this PR is ready for another pass. Please take a look thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this feedback is straightforward, except the very last item regarding subprocess.run
. I'm happy to go over that in our sync tomorrow morning if my comment isn't clear.
@kdmccormick Thanks for reviewing the PR, I have fixed all your comments, and the PR is ready for another pass. |
Taking another look... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a partial review-- I'll have some more comments tomorrow, and I still need to run each check locally. But generally this is looking good and it's exciting to see how much you were able to delete.
@kdmccormick I have fixed the comments. Could you take another look thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scripts/eslint.py
Outdated
def fail_quality(name, message): | ||
""" | ||
Fail the specified quality check. | ||
""" | ||
print(name) | ||
print(message) | ||
sys.exit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written, this displays the word "FAILURE" but it not actually cause the quality check to fail. That is problem... you can check the CI logs on this PR, and see how we are violating the threshold but still getting a green build:
(separate comment: looks like the violations limit needs to be 1303 rather than 1213)
Are you familiar with the idea of exit codes? It is a universal convention where each process in a system returns an integer upon completion. Zero indicates success, nonzero indicates failure. That is how GitHub Actions and many other systems determine whether a process succeeded or not. When you raise a Python exception, that automatically causes the process to return a nonzero exit code.
Here, sys.exit()
returns exit code 0, indicating success. Obviously, that is not what we want :) You want to raise exit code 1, indicating "general error" to the surrounding process, which will cause the PR check to fail as desired.
scripts/eslint.py
Outdated
"--ext", ".js", | ||
"--ext", ".jsx", | ||
"--format=compact", | ||
"." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"." | |
"lms", | |
"cms", | |
"common", | |
"openedx", | |
"xmodule", |
I haven't tested it yet, but perhaps specifying each root path would lower the violation count. Specifying "."
(current directory) might be including node_modules accidentally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdmccormick I believe your thoughts are right, making these changes the error count reduced to 1285 from 1303.
result = subprocess.run( | ||
command, | ||
text=True, | ||
check=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check=False, | |
check=True, |
Using check=True ensures that the script will fail if subprocess.run
fails. Using check=False hides the errors-- in other words, check=False means "fail silently".
Failing silently is usually bad. In CI scripting, failing silently is very bad, because it means that PRs can pass tests even when they shouldn't.
@salman2013 , every time you call subprocess.run
, including here, please please use check=True unless you are actively handling the error case yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdmccormick
You are right, actually, i intentionally used check=False
, Reason is that check-True
command is failing with status code 1. I tried to figure out the reason, there could be the following three possibility
- Command Syntax
- Eslint Configuration issue
- Files path issue
The above three issues were not the reasons i investigated. The result.stderr
is showing nothing but the result.stdout
is printing almost 1285 errors from different files. Which i figured out the command is sending status code 1 as a lot of errors in different files.
As we raise exceptions in case of violation count comparison, I thought it safe to use check=False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see @salman2013 . You are right, it makes sense to check=False
here. Just please then inspect the actual integer value of the exit code. According to the eslint docs, exit code 1 indicates that linting happened successfully but there were violations, whereas exit code 2 indicates that an unexpected error occurred (in which case we'd want to fail CI).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@salman2013 Could you open a PR to check that the exit code is 1? I have concern that an unexpected issue could one day cause eslint
to fail with an exit code of 2 or higher. As long as there are any digits on the last line of output, then the script would coincidentally "parse" those digits as the violation count. For example, an error like this like:
ERROR: Got 2 arguments, expected 1
might be parsed as "2" violations.
a61d9a7
to
ca2712a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used these PRs to confirm that each kind of build failure is caught by the check as we'd expect. Great work!
Just one comment about raising the pii_check threshold, and there is my comment from earlier about xsslint exit codes 1 vs 2. With those comments resolved, feel free to merge 🚀
Nitpick: Can you label this commit as build:
rather than chore:
? "chore" is meant to indicate minor, repetitive tasks like routine requirement upgrades. This PR is an overhaul of the quality and JS build suite, much more ambitious than a chore 😄
@@ -1,7 +1,7 @@ | |||
source_path: ./ | |||
report_path: pii_report | |||
safelist_path: .annotation_safe_list.yml | |||
coverage_target: 94.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PII annotation coverage has improved even further since my last review! Can you bump this up to 85.3?
scripts/eslint.py
Outdated
@@ -0,0 +1,73 @@ | |||
""" # pylint: disable=django-not-configured | |||
Check code quality using pycodestyle, pylint, and diff_quality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this comment before merging.
4788953
to
f85d452
Compare
🎉 Congrats, this has been a lot of work! |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Background: We have a large number of standing eslint violations in our legacy frontends. Rather than fixing or amnesty-ing all of them, we've opted to use a simple integer limit of violations. Exceeding this limit causes the quality check to fail. Before we moved eslint off of Paver [1], the limit was unreasonably high (probably due to deprecations that removed violation-rich frontend code). This was good, except for the fact that we essentially weren't catching when new violations creeped into the JS code. So, in [1], we lowered the limit down to the lowest possible value, which we thought was 1285. However, we've found that this made the check flaky-- turned out, we have been unintentionally double-counting various violations due to the symlinks in edx-platform. Some of those symlinks' existence is dependent on whether and how `npm ci` and `npm run build` have been run. As a result, 1285 would pass in some contexts, and fail in other contexts. The fix is to simply add all the relevant edx-platform symlinks to .eslintignore. This allows us to lower the violations limit to 734. [1] openedx#35159
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
Background: We have a large number of standing eslint violations in our legacy frontends. Rather than fixing or amnesty-ing all of them, we've opted to use a simple integer limit of violations. Exceeding this limit causes the quality check to fail. Before we moved eslint off of Paver [1], the limit was unreasonably high (probably due to deprecations that removed violation-rich frontend code). This was good, except for the fact that we essentially weren't catching when new violations creeped into the JS code. So, in [1], we lowered the limit down to the lowest possible value, which we thought was 1285. However, we've found that this made the check flaky-- turned out, we have been unintentionally double-counting various violations due to the symlinks in edx-platform. Some of those symlinks' existence is dependent on whether and how `npm ci` and `npm run build` have been run. As a result, 1285 would pass in some contexts, and fail in other contexts. The fix is to simply add all the relevant edx-platform symlinks to .eslintignore. This allows us to lower the violations limit to 734. [1] openedx#35159
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Background: We have a large number of standing eslint violations in our legacy frontends. Rather than fixing or amnesty-ing all of them, we've opted to use a simple integer limit of violations. Exceeding this limit causes the quality check to fail. Before we moved eslint off of Paver [1], the limit was unreasonably high (probably due to deprecations that removed violation-rich frontend code). This was good, except for the fact that we essentially weren't catching when new violations creeped into the JS code. So, in [1], we lowered the limit down to the lowest possible value, which we thought was 1285. However, we've found that this made the check flaky-- turned out, we have been unintentionally double-counting various violations due to the symlinks in edx-platform. Some of those symlinks' existence is dependent on whether and how `npm ci` and `npm run build` have been run. As a result, 1285 would pass in some contexts, and fail in other contexts. The fix is to simply add all the relevant edx-platform symlinks to .eslintignore. This allows us to lower the violations limit to 734. [1] #35159
Background: We have a large number of standing eslint violations in our legacy frontends. Rather than fixing or amnesty-ing all of them, we've opted to use a simple integer limit of violations. Exceeding this limit causes the quality check to fail. Before we moved eslint off of Paver [1], the limit was unreasonably high (probably due to deprecations that removed violation-rich frontend code). This was good, except for the fact that we essentially weren't catching when new violations creeped into the JS code. So, in [1], we lowered the limit down to the lowest possible value, which we thought was 1285. However, we've found that this made the check flaky-- turned out, we have been unintentionally double-counting various violations due to the symlinks in edx-platform. Some of those symlinks' existence is dependent on whether and how `npm ci` and `npm run build` have been run. As a result, 1285 would pass in some contexts, and fail in other contexts. The fix is to simply add all the relevant edx-platform symlinks to .eslintignore. This allows us to lower the violations limit to 734. [1] #35159
Paver is deprecated, and we aim to fully remove it soon. You can find more details here:
Paver deprecation issue
Paver removal PR
A few Paver commands remain in the edx-platform, and to proceed with its removal, we've replaced the following Paver commands with equivalent Make commands:
How to Test This PR:
In the edx-platform directory, you can use the following terminal commands to check for linting issues in Python and JavaScript files. Make sure your virtual environment is activated before running these commands.
make pycodestyle
make eslint
make stylelint
make xsslint
make pi_check
make check_keyword
make test-js
make test-coverage
make quality-test
For more details, please refer to the ticket: Replace paver quality and js_test commands