Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Pull requests tests documentation for contributors #8483

Merged
merged 6 commits into from
Jan 20, 2021

Conversation

sivaschenko
Copy link
Member

@sivaschenko sivaschenko commented Jan 7, 2021

Purpose of this pull request

This pull request (PR) introduces information for contributions on working with pull request tests, launching them, viewing test results and linking related pull requests for testing

Affected DevDocs pages

whatsnew
Added a new topic describing how contributors can work with pull request tests.

@lenaorobei
Copy link
Contributor

Overall, the content looks good to me.

However, I noticed that the most of the information that is related to the contribution flow is located here - https://devdocs.magento.com/contributor-guide/contributing.html
Probably, it make sense to move sections like "Magento contributor assistant" or "Forks and pull requests" to child pages as well.

Copy link
Member

@slavvka slavvka left a comment

Choose a reason for hiding this comment

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

Please address my comments

@sivaschenko
Copy link
Member Author

@lenaorobei thatks for the review. I've added the link to the added page from pull request creation steps. I think it's a good idea to break currently huge Contribution Guide to several pages for better readability, however, I think it would be better to do in a separate pull request. The goal of this one is to provide missing documentation on pull request tests

sivaschenko added a commit to sivaschenko/devdocs that referenced this pull request Jan 19, 2021
@sivaschenko sivaschenko changed the title [WIP] Pull requests tests documentation for contributors Pull requests tests documentation for contributors Jan 19, 2021
@sivaschenko sivaschenko marked this pull request as ready for review January 19, 2021 21:45
@sivaschenko
Copy link
Member Author

@slavvka thanks for the review! I have removed extra details from the description of the builds and kept more general tests description (see responses to your comments). Can you please review the PR again?

@sivaschenko sivaschenko requested a review from slavvka January 19, 2021 21:46
@jeff-matthews jeff-matthews added Internal Dev Differentiates work between community and Magento staff New Topic A major update published as an entirely new document labels Jan 20, 2021
sivaschenko and others added 2 commits January 20, 2021 20:48
Co-authored-by: Jeff Matthews <matthews.jeffery@gmail.com>
@jeff-matthews
Copy link
Contributor

running tests

@jeff-matthews
Copy link
Contributor

Jenkins failed on link checking:

+ rake test:html
�[35mChecking HTML ...�[0m
Running ["ScriptCheck", "ImageCheck", "HtmlCheck", "LinkCheck", "LinkChecker::DoubleSlashCheck"] on ["_site"] on *.html... 


Ran on 2647 files!


- internally linking to /contributor-guide/pull-request-tests, which does not exist
  *  _site/contributor-guide/contributing.html (line 581)
     <a href="/contributor-guide/pull-request-tests">pull request tests</a>
- internally linking to /contributor-guide/pull-request-tests/, which does not exist
  *  _site/contributor-guide/backward-compatible-development/index.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
  *  _site/contributor-guide/contributing.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
  *  _site/contributor-guide/contributing_dod.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
  *  _site/contributor-guide/contributors.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
  *  _site/contributor-guide/maintainers.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
  *  _site/contributor-guide/pull-request-tests.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
  *  _site/contributor-guide/templates/basic_template.html (line 262)
     <a href="/contributor-guide/pull-request-tests/" class="spectrum-SideNav-itemLink ">Pull Request Tests</a>
rake aborted!
HTML-Proofer found 8 failures!

I'm checking locally to see what the problem is.

Copy link
Contributor

@jeff-matthews jeff-matthews left a comment

Choose a reason for hiding this comment

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

Adding the .html file extension to the new topic references appears to fix the link errors even though the links seem to work without the extension.

@dshevtsov, is this a configuration issue with the htmlproofer tool?

@dshevtsov
Copy link
Collaborator

Adding the .html file extension to the new topic references appears to fix the link errors even though the links seem to work without the extension.

@dshevtsov, is this a configuration issue with the htmlproofer tool?

I'm not sure I understand the question. The system works as expected. We do not support implicit conversion of /dir/page.html onto /dir/page/.

@jeff-matthews
Copy link
Contributor

Link check fails unless the link contains the file extension for the new topic. However, similar links to other topics do not fail. See the TOC entry for backward incompatible development:

- label: Backward compatible development
          url: /contributor-guide/backward-compatible-development/
          versionless: true

That seems like odd behavior for our link checking tool.

@dshevtsov
Copy link
Collaborator

dshevtsov commented Jan 20, 2021

Link check fails unless the link contains the file extension for the new topic. However, similar links to other topics do not fail. See the TOC entry for backward incompatible development:

- label: Backward compatible development
          url: /contributor-guide/backward-compatible-development/
          versionless: true

That seems like odd behavior for our link checking tool.

The url /contributor-guide/backward-compatible-development/ works because the source topic is located in the /contributor-guide/backward-compatible-development/index.md file. In this case, both /contributor-guide/backward-compatible-development/ and /contributor-guide/backward-compatible-development/index.html would work.

Co-authored-by: Jeff Matthews <matthews.jeffery@gmail.com>
@jeff-matthews
Copy link
Contributor

Correct. I still think it's odd that htmlproofer considers one a failure and not the other.

@sivaschenko
Copy link
Member Author

@jeff-matthews I've applied the link fix, thanks

@jeff-matthews jeff-matthews enabled auto-merge (squash) January 20, 2021 21:48
@jeff-matthews
Copy link
Contributor

running tests

@dshevtsov
Copy link
Collaborator

Correct. I still think it's odd that htmlproofer considers one a failure and not the other.

The link checker reproduces exact behavior on the web server:

  • /contributor-guide/backward-compatible-development/ works
  • /contributor-guide/backward-compatible-development/index.html works
  • /contributor-guide/pull-request-tests/ would return 404

@jeff-matthews
Copy link
Contributor

jeff-matthews commented Jan 20, 2021

When I build locally, the extensionless URL to the new topic works. I haven't tried it on staging.

@jeff-matthews jeff-matthews merged commit a0b7bc6 into magento:master Jan 20, 2021
@ghost
Copy link

ghost commented Jan 20, 2021

Hi @sivaschenko, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@dshevtsov
Copy link
Collaborator

When I build locally, the extensionless URL to the new topic works. I haven't tried it on staging.

Ah, good to know.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Internal Dev Differentiates work between community and Magento staff New Topic A major update published as an entirely new document Progress: done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants