scripts/test.sh: only test added pages or scripts #12112

vitorhcl · 2024-01-23T15:44:47Z

The page(s) are in the correct platform directories: common, linux, osx, windows, sunos, android, etc.
The page(s) have at most 8 examples.
The page description(s) have links to documentation or a homepage.
The page(s) follow the content guidelines.
The PR title conforms to the recommended templates.

This PR modify test.sh so that, if $COMMIT == true, the tests are only ran on files that were added to commit. It also modify the pre-commit hook.

This greatly improves the commit test, only testing files that weren't tested yet, i.e., only files that will be in fact modified by the commit, and not the entire repository 🎉

You can still run npm test without environment variables to test all files, though.

.husky/pre-commit

scripts/test.sh

Co-authored-by: Sebastiaan Speck <12570668+sebastiaanspeck@users.noreply.github.com>

sebastiaanspeck · 2024-01-24T05:23:16Z

scripts/test.sh

@@ -14,14 +14,39 @@ function exists {
  command -v "$1" >/dev/null 2>&1
 }

+function check_for_unstaged_changes {


I do not think this is something we want. It is a test script. Keep it clean and only for the purpose it is meant for

This can potentially cause some imprecisions when an already added file is modified, so I see some options:

Continue to do it interactively

Ignore this possibility and assume the user knows it

Warn the user in some way
2.1 Warn in the docs
2.2 Warn in every execution
2.3 Warn only when there are unstaged changes
2.4 Warn only when unstaged and staged files conflict

Take a specific action for the user (whenever there is unstaged changes OR if they conflict with staged ones)
4.1 Stash the unstaged changes
4.2 Stash the unstaged changes and pop the stash after the checks
4.3 Discard the unstaged changes
4.4 Add the unstaged changes

Throw an error under a specific condition
5.1 Throw an error if there are any unstaged changes
5.2 Throw an error if the unstaged changes conflict with staged ones

Let the user configure the action to be taken before the test is executed
6.1 Configure through a config file
6.2 Configure through an environment variable
6.2 Configure through an option

The script itself was originally intended to be used in the CI, so it is a nice catch you can also run it locally. I personally do not want git stuff like this in the test script.

Well, there is a CI variable already. Maybe we could split out the test scripts to keep the CI script minimal.

I would agree. We don't want to check for unstaged changes here. If you have another use-case for interactive testing, this should be a separate script that is separately documented.

tldr-bot · 2024-02-02T03:13:46Z

The build for this PR failed with the following error(s):

scripts/test-ci.sh: line 20: run_pages_tests: command not found

Please fix the error(s) and push again.

vitorhcl · 2024-02-02T03:16:41Z

.husky/pre-commit

@@ -1 +1 @@
-npm test
+COMMIT=true npm test


COMMIT=true needs to be passed directly because of Husky v9

Outdated

sbrl

Hey, thanks for the script update! I've got some comments below for you to review.

I do have an additional general question though. What happens in the case of a page edit? Do edited pages also get checked?

sbrl · 2024-02-06T21:37:07Z

scripts/test-non-ci.sh

+# This script is the entrypoint for test.sh.
+#
+# NOTE: must be run from the repository root directory to correctly work!
+# NOTE: `set -e` is applied conditionally only if needed.


Please document the purpose of this script more fully.

sbrl · 2024-02-06T21:37:45Z

scripts/test-non-ci.sh

+if [[ $stash_pop == 0 ]]; then
+  git stash pop
+fi


What is this stash command for?

See #12112 (comment) and the comments below it.

sbrl · 2024-02-06T21:39:48Z

scripts/test.sh

+  for script in $1; do
+    flake8 $script
+  done


I'm somewhat confused here. This implies an argument is now passed to this function. Where does it come from?

Yes, it only runs flake8 on the passed scripts, on test-non-ci.run_tests_commit, it only sends to run_flake8 the scripts that were modified

sebastiaanspeck · 2024-03-07T04:43:09Z

@vitorhcl any update on this one?

sebastiaanspeck · 2024-04-04T03:52:35Z

@vitorhcl sending a friendly nudge to remind you of this open PR.

vitorhcl · 2024-04-04T13:29:37Z

I do have an additional general question though. What happens in the case of a page edit? Do edited pages also get checked?

Any file that was modified or added gets checked.

sebastiaanspeck · 2024-05-18T04:07:04Z

@vitorhcl could you resolve the merge conflicts? After that we can merge it IMO

scripts/test.sh: only test added pages or scripts

1a99866

vitorhcl requested a review from sebastiaanspeck as a code owner January 23, 2024 15:44

github-actions bot added the tooling Helper tools, scripts and automated processes. label Jan 23, 2024

vitorhcl requested a review from kbdharun January 23, 2024 15:45

kbdharun requested a review from acuteenvy January 23, 2024 15:59

sebastiaanspeck reviewed Jan 23, 2024

View reviewed changes

.husky/pre-commit Show resolved Hide resolved

scripts/test.sh Outdated Show resolved Hide resolved

vitorhcl and others added 4 commits January 23, 2024 15:20

scripts/test.sh: remove additional space

38d8a6b

Co-authored-by: Sebastiaan Speck <12570668+sebastiaanspeck@users.noreply.github.com>

.husky/pre-commit: set $COMMIT to true and then run npm test

69e4fc9

Co-authored-by: Sebastiaan Speck <12570668+sebastiaanspeck@users.noreply.github.com>

scripts/test.sh: only run black for files passed as argument

ce98225

scripts/test.sh: offer actions when there are unstaged changes

8817406

sebastiaanspeck previously requested changes Jan 24, 2024

View reviewed changes

kbdharun requested a review from MasterOdin January 24, 2024 15:54

vitorhcl added 2 commits February 1, 2024 21:02

scripts/test.sh: uncommment functions on run_tests

9ca8c8b

scripts/test.sh: check if CI is true before calling python tests

04afa76

vitorhcl force-pushed the test-only-added-files branch from 8de33b6 to 04afa76 Compare February 2, 2024 00:21

vitorhcl and others added 2 commits February 1, 2024 23:28

Merge branch 'main' into test-only-added-files

43ca64f

scripts: split common, non-ci and ci functions

601476e

vitorhcl requested a review from sbrl as a code owner February 2, 2024 03:13

vitorhcl commented Feb 2, 2024

View reviewed changes

vitorhcl requested a review from sebastiaanspeck February 2, 2024 03:16

scripts: fix test-ci.sh function

901fa56

sebastiaanspeck added the review needed Prioritized PRs marked for reviews from maintainers. label Feb 2, 2024

sbrl requested changes Feb 6, 2024

View reviewed changes

vitorhcl added 2 commits April 4, 2024 10:32

test-non-ci.sh: improve documentation

2ab2b48

scripts/test.sh: improve documentation

8e55301

vitorhcl requested review from sbrl, sebastiaanspeck and kbdharun and removed request for sebastiaanspeck and kbdharun April 4, 2024 13:33

sebastiaanspeck approved these changes Apr 4, 2024

View reviewed changes

Merge branch 'main' into test-only-added-files

22a53b8

kbdharun approved these changes Apr 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts/test.sh: only test added pages or scripts #12112

scripts/test.sh: only test added pages or scripts #12112

vitorhcl commented Jan 23, 2024

sebastiaanspeck Jan 24, 2024

vitorhcl Jan 24, 2024 •

edited

sebastiaanspeck Jan 24, 2024 •

edited

vitorhcl Jan 24, 2024

sbrl Feb 6, 2024

tldr-bot commented Feb 2, 2024

vitorhcl Feb 2, 2024

sbrl left a comment

sbrl Feb 6, 2024

sbrl Feb 6, 2024

vitorhcl Mar 7, 2024

sbrl Feb 6, 2024

vitorhcl Mar 7, 2024

sebastiaanspeck commented Mar 7, 2024

sebastiaanspeck commented Apr 4, 2024

vitorhcl commented Apr 4, 2024

sebastiaanspeck commented May 18, 2024

scripts/test.sh: only test added pages or scripts #12112

Are you sure you want to change the base?

scripts/test.sh: only test added pages or scripts #12112

Conversation

vitorhcl commented Jan 23, 2024

Choose a reason for hiding this comment

vitorhcl Jan 24, 2024 • edited

Choose a reason for hiding this comment

sebastiaanspeck Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tldr-bot commented Feb 2, 2024

Choose a reason for hiding this comment

sbrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastiaanspeck commented Mar 7, 2024

sebastiaanspeck commented Apr 4, 2024

vitorhcl commented Apr 4, 2024

sebastiaanspeck commented May 18, 2024

vitorhcl Jan 24, 2024 •

edited

sebastiaanspeck Jan 24, 2024 •

edited