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

Patch better rst test #602

Merged
merged 5 commits into from Jun 26, 2017

Conversation

Projects
None yet
2 participants
@mscuthbert
Copy link
Contributor

mscuthbert commented Jun 5, 2017

Tests for actual presence of correctly formatted output rather than
just some metadata. As shown so patiently by @mpacer in #601.

mscuthbert added some commits Jun 5, 2017

Much better test for RST metadata output
Tests for actual presence of correctly formatted output rather than
just some metadata. As shown so patiently by @mpacer in #601.
@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 5, 2017

Hey, this is looking great! This should definitely work for this particular case, but in the vein of more robust testing, it seems that where what we had before was expecting too little in the way of specifics, this is too much in the way of specifics. I.e., there are things we might add to the rst image metadata someday and if it's possible we should write the test so that it fails only if what we're adding actually is invalid and breaks the behaviour of setting the image metadata.

Some thoughts with this :

  1. since you are explicitly using and checking the content of result, you probably don't need to explicitly test that it's not None (it's not an expensive operation but I thought I might as well point that out)
  2. won't this fail if it has a non-numerical metadata value before the :width: tag?
  3. is it only valid to specify the width and height in pixels?
  4. Won't this fail if you specify the height first and the width second?
    • one thought would be to grab everything with the pattern :.*?: \w+?\n using a non-capturing grouping operation (?:) to apply a * to the whole bunch followed an arbitrary amount of spaces and another newline (so something like ((?::.*: \w+?\n)*?)\s*\n) that way all of the metadata objects that you might want are captured in a single group.
  5. You probably don't need to assign the value of result.group(x) to attr_string, you can just check directly assert ":width:" in result.group(1) (I know I did… but you don't have to)
    • alternatively, it may be somewhat clearer if the two groups did have different meaning to assign them specifically and assert that the elements are present (e.g., width_string = result.group(1) height_string = result.group(2)) but in this case they don't have different syntax/semantics so it may not be clearer to do that

NB: Since we're using this as an exploration of the codebase and testing converted documents, I'm being trying to give a bit more care than you would want to give to the average test. If you want me to ask for less, I'm happy to oblige.

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 6, 2017

Ah, okay -- no problem with patching it. In music21 we tend to do a fail-fast philosophy where if more metadata classes or something are added later we want the test to fail to make sure that this is reflected in the tests. I also tend to do very general tests first, like checking that output exists (since checking for None is basically a zero-time operation) so that I know immediately how bad the failure is (was there no output whatsoever or was it in an incorrect format).

For PNG testing, I think that px is going to be the only valid format, but that's easily removed.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 22, 2017

And that's probably a good point about the cheapness of checking for None & the use of general tests to know how bad a test is.

I think it's fair to have width be required to be a pixel value for now. Technically rst allows a percentage as the :width: value, where that implies it's the percentage of the line width inside of which the image is being displayed. But nothing's using that right now including the IPython display mechanism, so I think it's fine not to code for that now.

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 22, 2017

Great so I think it should be good to go if you think so.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 23, 2017

I've been trying to figure out why that error is occurring for py<=3.5 by looking at the source, docs, and issues and I'm a little stumped. I'll dive into it in a bit.

@mscuthbert mscuthbert closed this Jun 23, 2017

@mscuthbert mscuthbert reopened this Jun 23, 2017

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 23, 2017

closing and reopening to rerun the tests and see if it was a problem with this patch or something on the nbconvert master at the time.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 23, 2017

I can identify the source of the error: you're actually using a python 3.6 only feature

(?imsx-imsx:...)
(Zero or more letters from the set 'i', 'm', 's', 'x', optionally followed by '-' followed by one or more letters from the same set.) The letters set or removes the corresponding flags: re.I (ignore case), re.M (multi-line), re.S (dot matches all), and re.X (verbose), for the part of the expression. (The flags are described in Module Contents.)

New in version 3.6.

Which explains why it only works in 3.6 :P

Unfortunately, it means that we can't use this feature as nbconvert will need to be compatible with py2 for now.

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 23, 2017

Ah! I didn't see that -- I'll get a patch in that doesn't use that fancy Py3.6 feature soon as I can.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 23, 2017

An aside: I didn't know that that was a feature, so thank you for giving me one more reason to love being on the latest python.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 23, 2017

Sounds good!

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 23, 2017

That and having the Regexp flags available as an IntEnum -- this will be great when 3.6 can be assumed a few years from now.

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 24, 2017

python3.4 test failure because out of sockets -- try second time...

@mscuthbert mscuthbert closed this Jun 24, 2017

@mscuthbert mscuthbert reopened this Jun 24, 2017

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented Jun 24, 2017

The new Regex seems to work on all versions -- the first test had a py3.4 failure which had to do with being out of sockets -- this seems to be a test system failure; it did not occur in rst conversion, and a second run of the tests worked on all systems.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jun 26, 2017

w00t, then I'll merge it! Thank you!

@mpacer mpacer merged commit eec0a5e into jupyter:master Jun 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpacer mpacer modified the milestone: 5.3 Aug 1, 2017

@mpacer mpacer added unlogged and removed unlogged labels Aug 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment