openedx
Replace Paver quality and js_test commands
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:
- Replaced paver run_pep8 with pycodestyle
- Replaced paver run_eslint
- Replaced paver run_stylelint
- Replaced paver run_xsslint
- Replaced paver run_pii_check
- Replaced paver check_keywords
- Replaced paver test_js* and paver diff_coverage
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
Thanks for the pull request, @salman2013!
What's next?
Please work through the following steps to get your changes ready for engineering review:
:radio_button: Get product approval
If you haven't already, check this list to see if your contribution needs to go through the product review process.
- If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
- If it doesn't, simply proceed with the next step.
:radio_button: Provide context
To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
- Dependencies
This PR must be merged before / after / at the same time as ...
- Blockers
This PR is waiting for OEP-1234 to be accepted.
- Timeline information
This PR must be merged by XX date because ...
- Partner information
This is for a course on edx.org.
- Supporting documentation
- Relevant Open edX discussion forum threads
:radio_button: Get a green build
If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.
:radio_button: Let us know that your PR is ready for review:
Who will review my changes?
This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.
Where can I find more information?
If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
- Overview of Review Process for Community Contributions
- Pull Request Status Guide
- Making changes to your pull request
When can I expect my changes to be merged?
Our goal is to get community contributions seen and reviewed as efficiently as possible.
However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
- The size and impact of the changes that it introduces
- The need for product review
- Maintenance status of the parent repository
:bulb: As a result it may take up to several weeks or months to complete a review and merge your PR.
@feanil @kdmccormick Should we remove the paver tests as well like https://github.com/openedx/edx-platform/tree/master/pavelib/paver_tests?
@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 https://github.com/openedx/edx-platform/pull/35159 is not only to get rid of the paver ... commands. The goal is get rid of all the Python code that runs our quality checks. In the end, we should be able to delete the whole pavelib directory.
Keep in mind that there are several things that pavelib code did that are now completely unnecessary, for example:
- junit.xml -- I don't know if we need to write this file any more. If nothing is obviously using it, it can be deleted.
- violation counts -- There was a time when all checks had "violation limits", so we would need to keep track of the violation count, and then fail only if the count exceeded the limit. For many checks, the violation limit is now zero. If the violation limit for a check is zero, then that check can be simplified, since any violation is enough to fail it.
I recommend starting with one simple check, such as pycodestyle, and see if you can get that running without any Python scripts involved.
@kdmccormick Could you please look into my comment https://github.com/openedx/edx-platform/issues/34845#issuecomment-2268856419
@kdmccormick This PR is ready for another pass. Please take a look thanks.
@kdmccormick I believe this PR is ready for another pass. Please take a look thanks.
@kdmccormick Thanks for reviewing the PR, I have fixed all your comments, and the PR is ready for another pass.
I've created a series of PRs based on this branch to make sure that each check fails when a violation is added:
- pycodestyle
- https://github.com/kdmccormick/edx-platform/pull/21
- pii_check
- https://github.com/kdmccormick/edx-platform/pull/18
- check_keywords
- https://github.com/kdmccormick/edx-platform/pull/19
- test-js
- https://github.com/kdmccormick/edx-platform/pull/20
- coverage-js
- https://github.com/kdmccormick/edx-platform/pull/22
- eslint
- https://github.com/kdmccormick/edx-platform/pull/24
- stylelint
- https://github.com/kdmccormick/edx-platform/pull/23
- xsslint
- https://github.com/kdmccormick/edx-platform/pull/25
@salman2013 I'm currently validating each individual check with this spreadsheet: https://docs.google.com/spreadsheets/d/1KHl4bD9z1OodgLsJLUkobL8R0itMikEOSbHmkIFsMok/edit?gid=0#gid=0
While testing this PR, I found an existing logging issue with our JS CI, and I made a fix that is ready for review: https://github.com/openedx/edx-platform/pull/35954
Once that merges, could you rebase on top of it (or merge it in) so that we have better JS test logging to work with?
@kdmccormick I have rebased this branch with https://github.com/openedx/edx-platform/pull/35954
@kdmccormick I have fixed the comments. Could you take another look thanks.
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.
2U Release Notice: This PR has been deployed to the edX production environment.
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.
2U Release Notice: This PR has been deployed to the edX production environment.