Reorganize "Testing" pages #415
Conversation
totten
force-pushed
the
master-test-intro
branch
from
5a59477
to
d9cd7f7
Compare
|
I like where this is going! I'm happy to review it more thoroughly once it's no longer |
|
@totten What's the status of this PR? Still WIP? What else does it need? I haven't looked at it yet. Just trying to whittle down the PR queue atm. |
totten
force-pushed
the
master-test-intro
branch
5 times, most recently
from
d6298be
to
89da098
Compare
As part of civicrm/civicrm-dev-docs#415 , the dev docs will assimilate and organize more of the instructions about testing.
As part of civicrm/civicrm-dev-docs#415 , the dev docs will assimilate and organize more of the instructions about testing.
As part of civicrm/civicrm-dev-docs#415 , the dev docs will assimilate and organize more of the instructions about testing.
As part of civicrm/civicrm-dev-docs#415 , the dev docs will assimilate and organize more of the instructions about testing.
|
@seanmadsen I think this is a mergeable revision now. In the summary table, I'm a little on the fence about whether to include the "Trade off" and "Good for" blurbs. Showing them makes the table a better summary; but hiding them makes the table a bit more approachable. |
Running civilint is important for any/all code. Seems a bit arbitrary emphasizing this point specifically for tests.
The second statement doesn't make much sense without the first statement. Put them together.
There are multiple tools for testing in JS. We'll want separate pages for each.
1. Add new entries for Protractor, Codeception, and QUnit 2. This menu includes (a) some pages about the overall testing process and (b) some pages about specific tools. Consistently put the general before the specific.
* Provide a better visualization of the the different test suites * Migrate setup instructions from https://github.com/civicrm/civicrm-core/blob/master/tests/README.md
I'd like to be able to merge the general, incremental improvements for the "Testing" section, but these two new pages aren't yet written. Comment them out.
totten
force-pushed
the
master-test-intro
branch
from
40c4b3e
to
2947590
Compare
|
Rebased to resolve conflict. |
|
I'll leave this open for comment until Friday and merge if there's no more issues. |
There was a problem hiding this comment.
@totten wow these are some fantastic additions! Thanks!
I requested a few small changes. Ready to merge when those changes are resolved.
redirects/internal.txt
Outdated
| core/develop tools/git | ||
| basics/filesystem framework/filesystem | ||
| core/architecture framework/codebase | ||
| framework/civimail/tokens https://docs.civicrm.org/user/en/latest/common-workflows/tokens-and-mail-merge/ | ||
| core/architecture framework/codebaseframework/civimail/tokens https://docs.civicrm.org/user/en/latest/common-workflows/tokens-and-mail-merge/ |
There was a problem hiding this comment.
This seems like a typo, combining these two lines. Can you fix or explain?
There was a problem hiding this comment.
Doh, that was a mistake in resolving the merge-conflict. Went too quickly on that. Fixing.
redirects/internal.txt
Outdated
| @@ -5,9 +5,11 @@ best-practices/documentation-style-guide documentation/style-guide | |||
| extensions/basics extensions | |||
| api/general api | |||
| hooks/hook_civicrm_trigger_info hooks/hook_civicrm_triggerInfo | |||
| testing/setup testing/index | |||
There was a problem hiding this comment.
I'm 95% sure that testing/setup testing/index needs to be changed to testing/setup testing in order for the redirect to work correctly. Would take more time to test this though because the redirects happen in the publisher app. Grumble. If you're comfortable acting on my 95% "hunch", then can you change to testing/setup testing?
There was a problem hiding this comment.
Sure, will give a try.
(Iftesting/setup testing doesn't work, we'll find out post-merge -- we can do a quick follow-up then.)
| @@ -1,5 +1,5 @@ | |||
| Documentation+Infrastructure+Canary develop | |||
| Book+style+guide best-practices/documentation-style-guide | |||
| CiviCRM+Unit+Testing+basic+information testing/setup | |||
There was a problem hiding this comment.
Same as above, can you remove /index here?
There was a problem hiding this comment.
I updated this (although it's not quite clear in the way Github highlights this line).
There was a problem hiding this comment.
I'm taking this as a lesson in Github mechanics -- when there's a line edit (removal+addition), you can comment on either the removal or the addition. It seems better to comment on the addition -- because (in that case) Github will automatically recognize that the code-review has been addressed.
There was a problem hiding this comment.
Ahh nice point! Noted.
Also worth mentioning that I really like the way GitLab allows you to "resolve" separate discussions manually within merge requests. Very slick.
| @@ -128,7 +128,7 @@ CiviRules+hooks hooks | |||
| Profile+Hooks hooks | |||
| Report+Hooks hooks | |||
| Tests+in+phpstorm tools/phpstorm/#testing | |||
| Testing testing/setup | |||
There was a problem hiding this comment.
Same as above, can you remove /index here?
docs/testing/index.md
Outdated
| <td> | ||
| <a href="phpunit">PHPUnit</a><br/> | ||
| <!-- <a href="codeception">Codeception</a><br/> --> | ||
| <s><a href="webtest">Selenium</a></s> |
There was a problem hiding this comment.
webtest is a broken link here. Can you fix?
|
Thanks for adjusting this Tim. Looks like everything is now resolved. Merging. |
As part of civicrm/civicrm-dev-docs#415 , the dev docs will assimilate and organize more of the instructions about testing.
As part of civicrm/civicrm-dev-docs#415 , the dev docs will assimilate and organize more of the instructions about testing.
Based on the evaluation/planning at the Devon sprint, we felt it was important to provide a better framing/summary of the kinds of tests that are available (and when to use them).
This PR reorganizes the content of the "Testing" chapter, migrates in content from
civicrm-coreREADMEs, and opens with a table to visualize the choice of testing techniques.It's planned to put Codeception and Protractor in here as well, but the for the moment I've disabled those links.
(Comment updated Oct 30.)