Add script to update expected folder for test sites #828

marvinchin · 2019-04-10T10:19:31Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain: workflow improvement

Resolves #761.

What is the rationale for this request?

Presently, developers have to update the expected output for the test sites by hand. As the number of test sites increase, rebuilding the test sites and updating the expected folders of each site becomes more unwieldy.

Being able to automatically updated the expected folders would help reduce the time spent on this process.

What changes did you make? (Give an overview)

Moved declaration of test site directory names to a separate file to reduce duplication
Add update.sh and update.bat which rebuilds all test sites and updates their expected folders.

Is there anything you'd like reviewers to focus on?

I don't have a Windows machine, so it would be great if a windows user can help to test both test.bat and update.bat to ensure they are behaving as expected 🙂

Testing instructions:

Run npm run updatetest or npm run updatetestwin. All test sites should be rebuilt, and the output in _site should be copied over to expected.

Proposed commit message: (wrap lines at 72 characters)

I've split the PR into 2 commits, each with their own commit message. Do let me know if I should change them!

yamgent

Tested it on Windows, works brilliantly. 👍

I got one request though, can update the maintainer's guide to mention about your new feature? Thanks! This part:

Rebuild the test files.

# on Unix
$ npm run updatetest

# on Windows
> npm run updatetestwin

luyangkenneth · 2019-04-10T15:31:32Z

I got one request though, can update the maintainer's guide to mention about your new feature? Thanks! This part:

@yamgent Seems like you forgot to include a link :P
Also, I think the relevant docs to update should be this section in the developer guide (instead of the maintainer guide)? -> https://markbind.org/devGuide/index.html#updating-tests

EDIT: Ok wait I see what you did there 👍 You can ignore this comment haha

luyangkenneth · 2019-04-10T15:39:31Z

test/functional/test_sites

@@ -0,0 +1,2 @@
+test_site
+test_site_algolia_plugin


Screenshot of the current docs:

Given that we no longer have one ("the") test site, it would be great to also update the details of this new testing approach in our developer guide (in the Running tests and Updating tests sections). In particular, mentioning that whenever a new test site is added, its folder name will need to be added to this file.

test/functional/test.bat

yamgent · 2019-04-11T10:36:41Z

I am foreseeing that the changes for the test.bat and test.sh is going to clash with #698 (specifically, in #698, one of the test sites actually builds a bit differently because we are testing the conversion feature).

Since #698 is a ~~big PR~~ (edit: big feature PR), could we wait for #698 to merge first before we come back to this PR? :)

marvinchin · 2019-04-11T10:52:12Z

Sure, we can revisit this after #698 is merged!

yamgent · 2019-04-12T04:17:56Z

@marvinchin #698 is merged, you can continue with resolving conflict now.

marvinchin · 2019-04-12T07:36:39Z

I've updated the scripts after rebasing on top of master. If someone could re-test the windows implementation again to ensure that it still works, that would be great!

@amad-person I made some changes to the implementation of test.sh to match the implementation of test.bat (see 1aab519) - do you mind taking a look at it to ensure that the behaviour is still as you had intended?

Thanks everyone 🙂

amad-person

Just a minor nit. The site_convert testing behaviour is working as intended in test.sh! Thanks for this PR 😄

test/functional/test.sh

luyangkenneth

LGTM; some ideas about phrasing and structure -

luyangkenneth · 2019-04-12T13:16:49Z

docs/devGuide/devGuide.md

@@ -136,20 +136,41 @@ $ npm run test
 For Windows users:

 ```
-$ npm run testwin
+> npm run testwin


💥
☁️
😮

luyangkenneth · 2019-04-12T13:22:51Z

docs/devGuide/devGuide.md

+> npm run updatetestwin
+```
+<box type="warning">
+  You should always check that the output generated is correct before committing your changes to the expected site.


Let's standardize on "expected files" and "test sites"? :D So if you were to use both terms together, you would phrase it as Update the expected files in all the test sites.

For this sentence, it might become something like: You should always check that the generated output is correct before committing any changes to the test sites.

luyangkenneth · 2019-04-12T13:41:57Z

docs/devGuide/devGuide.md


 ##### New features

-Add new site content into the `test/test_site/` folder to demonstrate the new feature. Ensure that the new content is included in the test site so that your feature will be tested when `markbind build` is run on the test site. Remember to update the expected HTML files in `test/test_site/expected/`.
+Add new site content into an existing test site or create a new test site to demonstrate the new feature. Ensure that the new content is included in the test site so that your feature will be tested when `markbind build` is run on the test site. Remember to update the expected HTML files for the test site.


I feel like there's some repetition here. The special case of adding new site content / creating a new test site only applies when adding a new feature.

So maybe we can remove the Changes to existing features section, and combine Simply / Remember to update the expected files... with L144-146.

Say, something like:

#### Updating tests Whether you are adding a new feature, updating existing features or fixing bugs, make sure to update the test sites to reflect the changes. You may use the following scripts to do this automatically: [...] #### When adding new features Add new site content into an existing test site or create a new test site to demonstrate the new feature. This is to ensure that your feature can be tested by building that test site. <box type"info"> When creating a new test site...

Also, dropping the HTML here because sometimes the non-HTML files get modified too

yamgent

Other than the documentation change suggestions by Kenneth, I got nothing else to add.

Declaration of test site directories are replicated across test.bat and test.sh. Let's move the declaration to a separate file and have the scripts obtain the directory names from the file instead.

The implementation of the bash and batch test script differs slightly. The batch script implementation allows multiple test convert sites, but the bash script implementation only allows a single test convert site. Let's update the bash script to allow it to be extensible to multiple test convert sites.

Developers have to update the expected folder for test sites by hand. Updating the expected folders for each site manually becomes especially unwieldy as the number of test sites increase. Let's have a script which automates the updating of expected folders in all of the test sites.

marvinchin · 2019-04-13T16:05:42Z

Updated the documentation as per the comments. Thanks again for looking at it!

marvinchin · 2019-04-13T16:15:25Z

Sorry, not sure what happened but the previous reviews seem to have been dismissed 🙁

acjh · 2019-04-13T16:25:43Z

Sorry, not sure what happened but the previous reviews seem to have been dismissed

Automatic dismissal of reviews via push is misrepresented in UI: isaacs/github#1157

luyangkenneth · 2019-04-13T17:15:05Z

docs/devGuide/devGuide.md

+$ npm run updatetest
+```
+
+For Windows users:


Maybe standardize to "On Unix" and "On Windows" like you mentioned in the maintainer guide?

The updatetest and updatetestwin scripts have been added for developers to update the expected output for all test sites. Also, testing instructions for the developer and maintainer guides assume that there is only one test site. Let's update the developer and maintainer guides to include usage instructions for the test site update scripts. Let's also generalize the testing instructions for multiple test sites.

yamgent · 2019-04-14T05:15:44Z

Sorry, not sure what happened but the previous reviews seem to have been dismissed 🙁

Automatic dismissal of reviews via push is misrepresented in UI: isaacs/github#1157

Understood the distress caused by this feature; I have disabled it, sorry!

yamgent · 2019-04-15T02:08:47Z

There was a bug in update.bat, but I fixed it in 9c7438d.

Otherwise, this PR has improved my quality of life when doing releases, and even indirectly spotted a bug, so nice job. 👍

luyangkenneth · 2019-04-15T02:29:49Z

Woohoo impacc 🎉 @marvinchin

yamgent reviewed Apr 10, 2019

View reviewed changes

luyangkenneth reviewed Apr 10, 2019

View reviewed changes

marvinchin force-pushed the update-test-sites-script branch from 9f68a56 to 4756e49 Compare April 10, 2019 16:43

yamgent previously approved these changes Apr 11, 2019

View reviewed changes

yamgent added this to the v2.1.2 milestone Apr 11, 2019

marvinchin added the s.OnHold label Apr 11, 2019

yamgent removed this from the v2.1.2 milestone Apr 12, 2019

yamgent removed the s.OnHold label Apr 12, 2019

marvinchin dismissed yamgent’s stale review via 9596ee0 April 12, 2019 07:33

marvinchin force-pushed the update-test-sites-script branch from 4756e49 to 9596ee0 Compare April 12, 2019 07:33

amad-person requested changes Apr 12, 2019

View reviewed changes

test/functional/test.sh Outdated Show resolved Hide resolved

marvinchin force-pushed the update-test-sites-script branch from 9596ee0 to f5ae0d4 Compare April 12, 2019 09:44

amad-person previously approved these changes Apr 12, 2019

View reviewed changes

luyangkenneth previously approved these changes Apr 12, 2019

View reviewed changes

yamgent previously approved these changes Apr 13, 2019

View reviewed changes

Marvin Chin added 2 commits April 13, 2019 23:52

Move declaration of test site directories to separate file

636870b

Declaration of test site directories are replicated across test.bat and test.sh. Let's move the declaration to a separate file and have the scripts obtain the directory names from the file instead.

marvinchin dismissed stale reviews from yamgent, luyangkenneth, and amad-person via 2689339 April 13, 2019 15:53

marvinchin force-pushed the update-test-sites-script branch 2 times, most recently from 2689339 to d5d4839 Compare April 13, 2019 15:58

marvinchin force-pushed the update-test-sites-script branch from d5d4839 to b01d932 Compare April 13, 2019 16:00

marvinchin force-pushed the update-test-sites-script branch from b01d932 to 80f059b Compare April 13, 2019 16:17

luyangkenneth reviewed Apr 13, 2019

View reviewed changes

marvinchin force-pushed the update-test-sites-script branch from 80f059b to 329b333 Compare April 13, 2019 17:16

yamgent approved these changes Apr 14, 2019

View reviewed changes

yamgent added this to the v2.1.2 milestone Apr 14, 2019

yamgent merged commit fe42fe1 into MarkBind:master Apr 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script to update expected folder for test sites #828

Add script to update expected folder for test sites #828

marvinchin commented Apr 10, 2019

yamgent left a comment

luyangkenneth commented Apr 10, 2019 •

edited

Loading

luyangkenneth Apr 10, 2019

yamgent commented Apr 11, 2019 •

edited

Loading

marvinchin commented Apr 11, 2019

yamgent commented Apr 12, 2019

marvinchin commented Apr 12, 2019 •

edited

Loading

amad-person left a comment

luyangkenneth left a comment

luyangkenneth Apr 12, 2019

luyangkenneth Apr 12, 2019 •

edited

Loading

luyangkenneth Apr 12, 2019 •

edited

Loading

luyangkenneth Apr 12, 2019

yamgent left a comment

marvinchin commented Apr 13, 2019

marvinchin commented Apr 13, 2019

acjh commented Apr 13, 2019

luyangkenneth Apr 13, 2019

marvinchin Apr 13, 2019

yamgent commented Apr 14, 2019

yamgent commented Apr 15, 2019

luyangkenneth commented Apr 15, 2019

		@@ -0,0 +1,2 @@
		test_site
		test_site_algolia_plugin

Add script to update expected folder for test sites #828

Add script to update expected folder for test sites #828

Conversation

marvinchin commented Apr 10, 2019

yamgent left a comment

Choose a reason for hiding this comment

luyangkenneth commented Apr 10, 2019 • edited Loading

luyangkenneth Apr 10, 2019

Choose a reason for hiding this comment

yamgent commented Apr 11, 2019 • edited Loading

marvinchin commented Apr 11, 2019

yamgent commented Apr 12, 2019

marvinchin commented Apr 12, 2019 • edited Loading

amad-person left a comment

Choose a reason for hiding this comment

luyangkenneth left a comment

Choose a reason for hiding this comment

luyangkenneth Apr 12, 2019

Choose a reason for hiding this comment

luyangkenneth Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

luyangkenneth Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

luyangkenneth Apr 12, 2019

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

marvinchin commented Apr 13, 2019

marvinchin commented Apr 13, 2019

acjh commented Apr 13, 2019

luyangkenneth Apr 13, 2019

Choose a reason for hiding this comment

marvinchin Apr 13, 2019

Choose a reason for hiding this comment

yamgent commented Apr 14, 2019

yamgent commented Apr 15, 2019

luyangkenneth commented Apr 15, 2019

luyangkenneth commented Apr 10, 2019 •

edited

Loading

yamgent commented Apr 11, 2019 •

edited

Loading

marvinchin commented Apr 12, 2019 •

edited

Loading

luyangkenneth Apr 12, 2019 •

edited

Loading

luyangkenneth Apr 12, 2019 •

edited

Loading