Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move&rename JS tests #5053

Merged
merged 17 commits into from
Feb 9, 2014
Merged

Move&rename JS tests #5053

merged 17 commits into from
Feb 9, 2014

Conversation

jdfreder
Copy link
Member

@jdfreder jdfreder commented Feb 7, 2014

This PR will be used to try different JS test arrangements and eventually agree on one or keep the all-test-in-one-folder design we have now.

@ivanov
Copy link
Member

ivanov commented Feb 7, 2014

you'll need to adjust the test_cases around line 184 of ipython/IPython/testing/iptestcontroller.py for the tests to start passing here

@ivanov
Copy link
Member

ivanov commented Feb 7, 2014

That's also the place to add test sub-groups for js tests under js_test_group_names, the name of the group gets passed to JSController, let me know if you get stuck or want some help with this

@jdfreder
Copy link
Member Author

jdfreder commented Feb 7, 2014

Thanks Paul! I'll take a look-

@jdfreder
Copy link
Member Author

jdfreder commented Feb 7, 2014

@ivanov What do you think of the setup I just pushed?

Instead of /casperjs/test_cases/*.js, there is now

/casperjs/js/*.js
/casperjs/notebook/*.js
/casperjs/widgets/*.js

And two new test groups notebook and widgets. The folder names == test group name, but I use an explicit if/elif block in iptestcontroller to map the group names to folder names. Maybe it would be better if this mapping was implicit? Or is that too magical? See here - jdfreder@a61cd87#diff-f7b20eb6befe7d2ed0f2b0a61f1b9c36R185

@ivanov
Copy link
Member

ivanov commented Feb 7, 2014

Sent you a PR to make the tests run- I think we should preserve 'js' to run all of the javascript tests, and rename the miscellaneous tests that are in the 'js' directory in your branch right now and call them 'jsmisc'

@ellisonbg
Copy link
Member

@ivanov we talked and decided that there should be a separate test group for each of the different subdirs in static: base, auth, notebook, widgets, tree. Each of these will eventually have a ton of tests and should be runnable by itself. We were thinking that the tests groups would be named js-base, etc.

Would it be difficult to allow those to be run separately, but also have a global js group that runs them all?

@jdfreder
Copy link
Member Author

jdfreder commented Feb 7, 2014

Woops, thanks for the PR.

@ivanov
Copy link
Member

ivanov commented Feb 7, 2014

having a js group that runs them all should be too hard, that's what I was just looking at...

@takluyver
Copy link
Member

Since "js" is already short, does it make sense to simply do them as folders under js, so you'd run e.g. iptest js/base? That also provides an obvious way to drill down further to individual files.

@ellisonbg
Copy link
Member

Sure, I think that syntax would be a nice way of doing it. I would love to have an easy way of running individual tests in subdirs.

@ivanov
Copy link
Member

ivanov commented Feb 7, 2014

@ellisonbg one can, just not from the way iptest uses it right now. Let me see if I can add that as well.
In the meantime, if you run casperjs directly, you can do this casperjs test --includes=util.js test/file/name.js so long as you have a notebook server running on 8888, otherwise, you specify --port as well

all casperjs/ subdirectories now treated as test categories
don't laugh. it's a serious problem.
@ivanov
Copy link
Member

ivanov commented Feb 7, 2014

I sent jdfreder#13 which supports running individual js tests, as well as treating all subdirectories of casperjs as a separate test group

@ivanov
Copy link
Member

ivanov commented Feb 8, 2014

Ok, I've updated the jdfreder#13 PR I sent @jdfreder

  • iptest js-subfolder will run the tests in that subfolder, and the subfolders aren't hard-coded.
  • iptest js-notebook/execute_code.js will run only that test file (like @ellisonbg wanted)
  • iptest js runs all subdirectories in casperjs (each with its own server, so the groups can be run in parallel)
  • the js- prefix is just one variable, so if we like js/subfolder style better, we can use that easily instead.

@ivanov
Copy link
Member

ivanov commented Feb 8, 2014

In my PR to @jdfreder I changed the prefix to js/ so now you do iptest js/notebook/execute_code.js, as per suggestion from @takluyver

@ellisonbg
Copy link
Member

I like this short form better, thanks @ivanov

On Fri, Feb 7, 2014 at 4:26 PM, Paul Ivanov notifications@github.comwrote:

In my PR to @jdfreder https://github.com/jdfreder I changed the prefix
to js/ so now you do iptest js/notebook/execute_code.js

Reply to this email directly or view it on GitHubhttps://github.com//pull/5053#issuecomment-34521797
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member

Hmm, getting a traceback:

IPython test group: js
Traceback (most recent call last):
  File "/Users/bgranger/Documents/Computing/IPython/code/ipython/IPython/testing/iptestcontroller.py", line 271, in do_run
    controller.launch()
  File "/Users/bgranger/Documents/Computing/IPython/code/ipython/IPython/testing/iptestcontroller.py", line 184, in launch
    test_cases = os.path.join(test_dir, self.section.split('-'))
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py", line 75, in join
    if b.startswith('/'):
AttributeError: 'list' object has no attribute 'startswith'

Also, I am not thrilled about the double nesting tests/casperjs/js/foo. Could we figure out a way to do just tests/js/foo. It would just mean putting utils.js in tests/js, but that should be fine as it doesn't actually run any tests so will just be skipped (we can still include it).

@ellisonbg
Copy link
Member

But let's get this done ASAP so other test work can continue.

@jdfreder
Copy link
Member Author

jdfreder commented Feb 8, 2014

It wont let me merge @ivanov 's PR because it hasn't been rebased with the changes to my branch. I'll check it out and merge it manually since he is probably offline right now.

@jdfreder
Copy link
Member Author

jdfreder commented Feb 8, 2014

Since the changes I had made were similar to @ivanov 's but not as progressed , I just force pushed his branch here.

@jdfreder
Copy link
Member Author

jdfreder commented Feb 8, 2014

I moved the tests into folders that map 1-to-1 one with the folders in static (as discussed in person with @ellisonbg ). Also, I remove tests\casperjs...

The structure follows:

  • tests\util.js
  • tests\base*.js
  • tests\notebook*.js
  • tests\services*.js
  • tests\tree*.js <- (I put the dashboard tests in here, is that right @ellisonbg ?)
  • tests\widgets*.js

The thought is that in another PR the main widgets folder will get moved out of the notebook/ folder in static (since the widgets don't event require any IPython specific elements to work).

@ellisonbg
Copy link
Member

There is an actual test failure on Travis. Something about js/__pycache__...

@jdfreder
Copy link
Member Author

jdfreder commented Feb 8, 2014

Just hardcoded it in to be ignored... I guess we wait on Travis now.

@@ -159,6 +159,18 @@ def launch(self):
self.cmd[2] = self.pycmd
super(PyTestController, self).launch()

js_prefix = 'js/'
Copy link
Member

Choose a reason for hiding this comment

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

Will this fail on Windows (/)?

@ellisonbg
Copy link
Member

The README.md in the tests folder should be updated with current information about how to run the JS tests.

There are a couple of places where we hardcoded the path separator /. Let's test this on Windows to make sure it works. there is one question though: obviously internally we should use os.sep instead of '/'. But what about the command line API: should it be:

  • iptest js/base for all platforms
  • iptest js/base for Unix and iptest js\base for Windows.

@ivanov
Copy link
Member

ivanov commented Feb 8, 2014

@takluyver and I verified that the glob stuff with a / works on
windows before putting that in. The js/ prefix is only used for
the command line interface, and is cut out before using the rest
as a path,, so that will work as well.

@ellisonbg
Copy link
Member

Nice, thanks @ivanov ! So I guess the only thing to fix then is the README.md.

@ellisonbg
Copy link
Member

I will rewrite the README.md in a separate PR. Everything else looks ready to go, merging.

ellisonbg added a commit that referenced this pull request Feb 9, 2014
@ellisonbg ellisonbg merged commit 52c3158 into ipython:master Feb 9, 2014
@jdfreder jdfreder deleted the js-test-refact branch March 10, 2014 18:42
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants