Correct STASH regular expression

[ehogan]

Attempts to fix #2624.

However, some of the test_pp_cf.py tests fail because the test files (located here) now have invalid STASH codes; 002000000000.44.101.131200.1920.09.01.00.00.b.pp and 008000000000.44.101.000128.1890.09.01.00.00.b.pp have a STASH code of 44101 (ValueError: Expected STASH code MSI string "mXXsXXiXXX", got u'm??s44i101') and st0fc942.b.pp and st0fc699.b.pp have a STASH code of 0 (ValueError: Expected STASH code MSI string "mXXsXXiXXX", got u'm02s00i???')

Would it be possible for someone to help? What should I do now?

[marqh]

hello @ehogan

the code lines

if msi_match is None:
    raise ValueError('Expected STASH code MSI string "mXXsXXiXXX", '

are being hit in these test cases

perhaps you could add a test, using one of these partial strings, e.g.

u'm??s44i101'

and compare behaviour of the current regular expression to your proposed change; perhaps there's a small behaviour tweak that you could implement

[ehogan]

Just one set of tests failed due to time out errors. Do the tests need to be restarted? I'm pretty sure everything is ok though. Thank you for the help @marqh! :)

[DPeterK]

Thanks for this update @ehogan - I've made a comment on something I'd like to see improved, but if you'd prefer not to make that improvement I quite understand!

[DPeterK]

So I realise that this isn't directly your problem as you've copied an existing pattern, it's just the pattern you've copied is quite unusual... What this is doing is using one of the unittest assertions to catch an error raised by another unittest assertion 😱

I would prefer to see something like the following in all of these cases:

test_val = 'mlotstmin'
exp_emsg = 'Expected STASH code .* {!r}'.format(test_val)
with self.assertRaisesRegexp(ValueError, exp_emsg):
    test_val == iris.fileformats.pp.STASH(1, 2, 3)

(give or take - I haven't tested this code!)

There are two changes here:

• I've removed the indented self.assertEqual and replaced it with a pure Python equality check
• I've replaced self.assertRaises with the preferred (and more robust) self.assertRaisesRegexp and added the expected regular expression failure. This tests that the right error has been raised, and that this test isn't throwing a false positive failure.

[DPeterK]

Ah - the tests have actually failed now rather than erroring and it looks like there are a couple of failing tests that need to be addressed before this can be merged...

[ehogan]

Thanks for the review @dkillick, all the tests are passing now :)

[DPeterK]

@ehogan good stuff 👍 I just need to check your most recent changes and we need to get you added to the contributors list, but neither of those things should be too arduous!

[djkirkham]

Are we sure we want to lose handling cases such as m1s1i1, where the numeric parts are longer or shorter than they should be?

[ehogan]

Thanks for the PR @djkirkham :) However, a lot of tests are failing :( (I'm not sure they're related to the changes in this branch though?)

[djkirkham]

@ehogan It's a problem with the testing environment (see #2769). As soon as there's a resolution for that I'll kick off the tests again and hopefully this can be merged.