cross platform: python script to run test files [suggestion] #951

obastemur · 2016-05-09T12:44:14Z

This PR

adds a Python based test script to run all the test files or the ones from
a given folder. In addition to executing JS files, it also compares the results
to predefined baseline files.
replaces the functionality of runtests.sh to call runtests.py

Try:

./build.sh
./test/runtests.py Basics

In order to run a single file

./test/runtests.py Basics/hello.js

obastemur · 2016-05-09T15:31:24Z

@dilijev I guess Copyright check fails because of py file?

obastemur · 2016-05-09T15:42:14Z

@dilijev I've updated netci.groovy to use runtests.py instead of runtests.sh (which is no longer exist) but somehow CI doesn't take this change into consideration?

dilijev · 2016-05-09T18:41:11Z

@obastemur Yes, changes to the CI script need to be merged in to the branch and then the tasks need to be regenerated before the CI will use them. netci.groovy is not used during the CI checks, it is only used to generate them. (That's part of why changes to the CI are so difficult to test.)

Probably the fastest thing to do for now is to have runtests.sh wrap runtests.py.

dilijev · 2016-05-09T18:42:37Z

@obastemur as for the copyright failure, yes, you can see in the logs:

test/runtests.py ... does not contain a correct Microsoft copyright notice.

dilijev · 2016-05-09T18:43:28Z

.gitignore

@@ -42,6 +42,7 @@ install_manifest.txt
 *.xcworkspace

 # additional *nix generated files
+/ch


Why /ch instead of just ch? Doesn't the leading slash only exclude the file if it is in the same directory as the .gitignore?

That's my intention. new build script creates a symbolic link for ch on the root. I want it to be ignored on git.

To be more general, we could ignore that file in any directory it happens to appear by simply removing the leading /

obastemur · 2016-05-09T18:47:47Z

Probably the fastest thing to do for now is to have runtests.sh wrap runtests.py.

and send an empty config to delete runtests.sh?

Do we have to do this? BTW; Where do we update that Copyright Check algorithm to cover py files ?

dilijev · 2016-05-09T18:47:48Z

test/runtests.py

@@ -0,0 +1,120 @@
+#!/usr/bin/env python


Yeah, I figured this would come up. Files that need shebangs will break the copyright check. This is arguably a bug in the copyright check that needs a special case. Previously I had specified that the copyright notice had to be the VERY FIRST thing in the file. But since semantically, the shebang needs to come first when it is present, I'll add an exception.
@digitalinfinity does this exception seem reasonable?

dilijev · 2016-05-09T19:00:43Z

@obastemur Why do you want to delete runtests.sh? It seems better to have a script which could run any test harness in any configuration and abstract the details of running the tests from the build definition?

Anyway changes to the CI script should be done as separate pull requests (and in a separate branch, as you've noted, to avoid disruption while we sort out the details).

dilijev · 2016-05-09T19:04:10Z

The copyright check files are:
https://github.com/Microsoft/ChakraCore/blob/master/jenkins/check_copyright.py
https://github.com/Microsoft/ChakraCore/blob/master/jenkins/check_copyright.sh

The fix to ignore a shebang line at the beginning of the file will be in
https://github.com/Microsoft/ChakraCore/blob/master/jenkins/check_copyright.py

obastemur · 2016-05-09T19:11:47Z

Why do you want to delete runtests.sh? It seems better to have a script which could run any test harness in any configuration and abstract the details of running the tests from the build definition?

Both are executable scripts, I don't see any reason to keep both.

Anyway changes to the CI script should be done as separate pull requests (and in a separate branch, as you've noted, to avoid disruption while we sort out the details).

Great. Gonna cherry-pick this to separate branch and let's see what's going to happen

jianchun · 2016-05-09T19:59:02Z

build.sh

+
+result=$?
+if [[ $result == 0 && ! -f ../ch ]]; then
+    ln -s BuildLinux/ch ../ch


I assume you did this change to save some typing while testing manually? I tried it out and this broke my personal settings. I have "." at head of PATH. When I build at repo root and run "ch", it prints

dlopen() failed; dlerror says 'ch: cannot open shared object file: No such file or directory'

The same happens if I have <repo>/BuildLinux at head of PATH.

My workaround was a wrapper script "ch" in my PATH to invoke BuildLinux/ch. That doesn't have this problem.

So maybe a top-level symbolic link to "ch" isn't very useful?

It has above problem. (maybe we can investigate and fix it.)

The primary binary is "chakracore", not "ch". "ch" is mostly a testing binary. A top-level link feels over kill for it.

We might consider build type subdirs like BuildLinux/x64_debug, BuildLinux/x64_release. Top-level ch link doesn't work with that.

I agree with the above concerns. It's fairly straightforward to know where the test binary is. Especially as long as this is intended to use with already-written test code, saving typing is a moot point. For manual testing, there's not that much extra typing to do, and a wrapper script might be more useful.

I tried it out and this broke my personal settings. I have "." at head of PATH. When I build at repo root and run "ch", it prints

local one is available at ./ch

I agree with the above concerns. It's fairly straightforward to know where the test binary is.

you can call ls -l ch

We might consider build type subdirs like BuildLinux/x64_debug,

We could have ch_d or ch_debug etc..

Especially as long as this is intended to use with already-written test code, saving typing is a moot point.

Sorry, didnt' understand. Could you please explain more?

obastemur · 2016-05-09T21:05:22Z

@jianchun ch is removed from root for now.
@dilijev see env is removed. Do you think that should cover?

Although I don't expect any problem from this PR, I'm still +1 on cherry-picking this PR to separate branch and run through CI first. However, I'm also okay if you don't see any reason for that too J

dilijev · 2016-05-09T21:07:46Z

@obastemur I still strongly oppose modifying netci.groovy as part of this change, or removing runtests.sh at all, until the test harness for linux becomes more mature. We might change mechanisms and having a single script which will automate whatever we need will remove churn from the netci.groovy file, which is difficult to manage, as you've seen. I think removing that script and making a change here is jumping the gun.

dilijev · 2016-05-09T21:08:54Z

jenkins/check_copyright.py

@@ -42,6 +42,8 @@ def report_incorrect(file_name, pairs):
 linecount = 0
 pairs = []
 with open(file_name, 'r') as sourcefile:
+    if not re.match(r'#!/usr/bin/env.*', sourcefile.readline(), 0):


Why not just sourcefile.readline().startswith("#!")? I think we're imposing too much structure on the shebang.

obastemur · 2016-05-09T21:24:42Z

CI related file changes are removed from the PR. I guess that's all ?

dilijev · 2016-05-09T21:24:58Z

test/runtests.sh

-# Eventual test running harness on *nix for ChakraCore
-# Today, it's simply there to make sure hello.js doesn't regress
-#
+# REMOVE THIS AFTER MERGING runtests.py in!


I don't think there's any need for this comment. We can make this decision later.

AFAIK the decision is done. Is there any technical reason to keep this file?

The idea is that, since netci.groovy is much slower to update and the way we run the tests could change, I'm ultimately trying to reduce resistance to changes by keeping this file as a wrapper for whatever mechanism we actually use to run tests. I think that's a fair enough technical reason (based on a technical limitation of the netci.groovy task generation mechanism).

I'm not aware of any decision regarding netci.groovy task changes or this file in particular. I'll start an email thread with @curtisman to sort out the details.

dilijev · 2016-05-09T21:25:53Z

Unless you include an exception to not return a failure error code for now, the linux tasks will keep failing until clang 3.8 is installed on the build machines, which should happen later today. Since that change is incoming, it's probably best to wait until it is done and then re-run the tests before merging this in.

This PR - adds a Python based test script to run all the test files or the ones from a given folder. In addition to executing JS files, it also compares the results to predefined baseline files. - adds a shebang support for copyright check - replaces runtests.sh on CI/linux Try; ''' ./build.sh ./test/runtests.py Basics ''' In order to run a single file ''' ./test/runtests.py Basics/hello.js '''

…op of scripts. Merge pull request #957 from dilijev:copy Unblocks copyright check on #951

dilijev · 2016-05-10T18:54:19Z

@dotnet-bot test Copyright Check please

dilijev · 2016-05-10T18:55:15Z

Following completion of the Copyright Check, LGTM and this is good to merge.

dilijev · 2016-05-11T18:52:13Z

Since the toolchain has been updated...
@dotnet-bot
test Ubuntu ubuntu_linux_debug please
test Ubuntu ubuntu_linux_release please

…ion] Merge pull request #951 from obastemur:testing This PR - adds a Python based test script to run all the test files or the ones from a given folder. In addition to executing JS files, it also compares the results to predefined baseline files. - replaces the functionality of `runtests.sh` to call `runtests.py` Try: ``` ./build.sh ./test/runtests.py Basics ``` In order to run a single file ``` ./test/runtests.py Basics/hello.js ```

msftclas added the cla-already-signed label May 9, 2016

dilijev reviewed May 9, 2016
View reviewed changes

jianchun reviewed May 9, 2016
View reviewed changes

dilijev reviewed May 9, 2016
View reviewed changes

dilijev mentioned this pull request May 9, 2016

Update check_copyright.py to ignore the #! line at the top of scripts. #957

Merged

dilijev added the Continuous Integration label May 10, 2016

chakrabot pushed a commit that referenced this pull request May 10, 2016

[MERGE #957] Update check_copyright.py to ignore the #! line at the t…

b4f8053

…op of scripts. Merge pull request #957 from dilijev:copy Unblocks copyright check on #951

chakrabot merged commit f45468c into chakra-core:linux May 12, 2016

obastemur deleted the testing branch June 7, 2016 17:48

cross platform: python script to run test files [suggestion] #951

cross platform: python script to run test files [suggestion] #951

Uh oh!

Conversation

obastemur commented May 9, 2016 • edited by dilijev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obastemur commented May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obastemur commented May 9, 2016

Uh oh!

dilijev commented May 9, 2016

Uh oh!

dilijev commented May 9, 2016

Uh oh!

dilijev May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev commented May 9, 2016

Uh oh!

dilijev commented May 9, 2016

Uh oh!

obastemur commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur commented May 9, 2016

Uh oh!

dilijev commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev commented May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dilijev commented May 10, 2016

Uh oh!

dilijev commented May 10, 2016

Uh oh!

dilijev commented May 11, 2016

Uh oh!

Uh oh!

obastemur commented May 9, 2016 •

edited by dilijev

Loading

obastemur commented May 9, 2016 •

edited

Loading

dilijev May 9, 2016 •

edited

Loading

dilijev May 9, 2016 •

edited

Loading

dilijev commented May 9, 2016 •

edited

Loading