Problem/Motivation
A small part of the parent issue with an easy to review set of changes. These test fixtures are similar to the d.o release history XML, https://updates.drupal.org/release-history/drupal/current
Steps to reproduce
Proposed resolution
Fixtures
find . -path "*core/*/fixtures/*/*.xml" -exec sed -i 's#http://example.com#https://example.com#g' {} \;
find . -path "*core/*/fixtures/*/*.xml" -exec sed -i 's#http://www.example.com#https://example.com#g' {} \;
And for the test files
find . -path "*core/modules/update/tests/src/Functional/\*Test*.php" -exec sed -i 's#http://example.com#https://example.com#g' {} \;
find . -path "*core/modules/update/tests/src/Functional/\*Test*.php" -exec sed -i 's#http://www.example.com#https://example.com#g' {} \;
Remaining tasks
Change the test files, per the second set of snippets above.
Review, for this issue a review is Novice level - The Drupal documentation has all the steps to review a merge request. A key part is to explain exactly what you did or did not do for the review. It is good practice for more complex issues where the reviewer may only review a part of a merge request.
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3562778
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 11.x
compare
- 3562778-replace-httpexample.com-with
changes, plain diff MR !14098
Comments
Comment #2
quietone commentedComment #3
garvitasakhrani commentedWorking on it.
Comment #5
garvitasakhrani commentedI’ve updated the test fixtures to replace http://example.com with https://example.com and pushed the changes to the issue fork branch 3562778-replace-httpexample.com-with.
Kindly review and let me know if any further changes are required.
Comment #6
smustgrave commentedThanks for working on this. Test failures seem relevant to the changes
Comment #7
quietone commented@garvitasakhrani, Welcome to Drupal! Thank you for working on this.
The changes made are what was asked for in the issue summary. Well done.
Now, unfortunately, there are failed tests which need to be resolved. Right now there is a red circle next to the link to MR for this issue at the end of the Issue Summary. If you click on that link, you get the an overview of all the tests that ran in our pipeline. Then, Select 'Tests' and scroll down the list to find which group of tests had failures. Here, it is 'PHPUnit Functional'. Select 'PHPUnit Functional' and the failed tests are shown at the top of the list. And selecting 'View details' does just that, it shows the error message and line where the failure occurred. I suggest you find time to explore the GitLab UI, if you are not already familiar with it. Testing is an integral part of Drupal core development.
As, for the failure, it is because the actual tests are still using 'http://' type URLs. So, those need to be changed as well. All the Functional tests are in the directory core/modules/update/test/src/Functional. So, we just need a new command to find those files and run sed. I have added those to the 'Proposed resolution' section of the Issue Summary'
Not thinking about the tests was an oversight on my part when I created this issue.
@garvitasakhrani, can you make the change to the test files?
Comment #8
garvitasakhrani commentedI fixed the test failures by replacing http://example.com with https://example.com in the remaining functional tests.
All CI pipelines are now passing.Kindly review and let me know if any further changes are required.
Thank you.
Comment #10
quietone commented@garvitasakhrani, thanks for the updates.
I applied the diff to 11.x HEAD, and searched for instances of http://example.com and http://www.example.com in the Update status module.
There are still instances of http://www.example.com in a Unit test but those are not using the test fixtures that are similar to the d.o release history XML, so technically out of scope. They will be done in another issue of the parent.
These changes are correct and in scope. Thanks @garvitasakhrani.
The next step here is a review. For this issue that is also novice level. I have updated the issue summary with information for that.
Comment #11
smustgrave commentedBeen about 3 weeks was hoping a new user would pick up the review but don't need the ticket to sit much longer.
Saved credit for garvitasakhrani and quietone.
My review was essentially the same as #10 and got the same. Also see the diff is 1 to 1 for additions/deletions.
LGTM.
Comment #12
longwaveCommitted and pushed 47c30496bb0 to main and 249bb60837c to 11.x. Thanks!