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

Fix bug when an AttributeError is raised, and add unit tests for xtrigger #3060

Merged
merged 4 commits into from
Apr 3, 2019

Conversation

kinow
Copy link
Member

@kinow kinow commented Apr 2, 2019

@dwsutherland, @hjoliver here are the unit tests for get_func, and that part of the SuiteConfig code that checks if the Xtrigger function is a callable.

One of the final unit tests was to check what would happen if we got an AttributeError, instead of an ImportError/ModuleNotFoundError. That is possible when you have a Xtrigger file cylc.py but without the cylc() function (I believe xtrigger module and function name are expected to match).

@dwsutherland I reckon this AttributeError would also cause the suite to hang? That error was propagated, instead of a SuiteConfigError, so I included a commit in this pull request to catch that error as well.

Now wait for Travis. Took a bit longer because decided to write using pytest instead of unittest the new test for cylc.config and because also decided to cover the AttributeError branch of get_func (second if statement in that function I think).

If approved, will prepare a back port for 7.8.x.

Cheers
Bruno

@kinow kinow added bug? Not sure if this is a bug or not small labels Apr 2, 2019
@kinow kinow added this to the cylc-8.0a1 milestone Apr 2, 2019
@kinow kinow self-assigned this Apr 2, 2019
@kinow kinow changed the title Fix bug when an AttributeError is raised, and add unit tests xtrigger Fix bug when an AttributeError is raised, and add unit tests for xtrigger Apr 2, 2019
@dwsutherland
Copy link
Member

dwsutherland commented Apr 2, 2019

Related back port of the fix is here:
#3059

And for some reason the Travis unit test is failing... No obvious error I can see

@cylc cylc deleted a comment Apr 2, 2019
@kinow
Copy link
Member Author

kinow commented Apr 2, 2019

@dwsutherland added onre more commit. Codacy was complaining about the assert statements. These assert statements are definitely bad in production, but they are the de-facto way of writing assertions in PyTest.

So added a .codacy.yml file excluding all our unit test folders (for cylc and for parsec), as well as non source code files (licences folder, etc folder, etc).

This should speed up Codacy, and prevent it of reporting issues in isodatetime, doc folder, etc. (even though most of these exclusions will disappear with time, as we progress towards Cylc 8, setup.py, etc)

@kinow
Copy link
Member Author

kinow commented Apr 2, 2019

Codacy is now happy! Just waiting for Travis now 🎉

@hjoliver
Copy link
Member

hjoliver commented Apr 2, 2019

(I believe xtrigger module and function name are expected to match).

yup

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Looks good! thanks for the tutorial, I need to write more of these 😁

@kinow kinow force-pushed the unit-tests-xtrigger branch from 3fdff38 to af6d060 Compare April 2, 2019 08:47
@kinow
Copy link
Member Author

kinow commented Apr 2, 2019

Thanks @dwsutherland ! And a FYI, I push-forced to edit the last commit, as the .codacy.yml was missing a break line after the last line with content. If anything blinks, it must be a flaky test and Travis will need a kick 👍

@kinow kinow force-pushed the unit-tests-xtrigger branch from af6d060 to 326ca51 Compare April 2, 2019 20:31
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hjoliver hjoliver merged commit c87597b into cylc:master Apr 3, 2019
@hjoliver
Copy link
Member

hjoliver commented Apr 3, 2019

If approved, will prepare a back port for 7.8.x.

Still need this @kinow ?

@kinow
Copy link
Member Author

kinow commented Apr 3, 2019

Yup @hjoliver , preparing one now. Was just waiting for David's other back port to go first. Will rebase my 7.8.x branch, cherry pick and prepare a backport for this one. Thanks!!

@kinow kinow mentioned this pull request Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug? Not sure if this is a bug or not small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants