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

use 'python -O' in 'eb' command (REVIEW) #1357

Merged
merged 20 commits into from
Feb 6, 2016
Merged

Conversation

boegel
Copy link
Member

@boegel boegel commented Aug 14, 2015

This is interesting because of significant performance improvements for (mainly) the logger via hpcugent/vsc-base#182, but needs to be evaluated carefully to make sure this doesn't break anything.

First step is to see whether all the unit tests still pass under python -O.

See also http://stackoverflow.com/questions/2055557/python-optimized-mode .

edit: now requires hpcugent/vsc-base#188

@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1970/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1970/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Aug 20, 2015

TODO:

  • fix broken tests
  • add support for eb --test, to make sure unit tests are run exactly like eb is (e.g., with python -O)
  • update documentation on running unit tests

@boegel boegel added this to the v2.4.0 milestone Sep 18, 2015
@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2059/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@@ -65,7 +65,7 @@ def test_easybuilderror(self):
self.assertErrorRegex(EasyBuildError, 'BOOM', raise_easybuilderror, 'BOOM')
logToFile(tmplog, enable=False)

log_re = re.compile("^%s :: BOOM \(at .*:[0-9]+ in [a-z_]+\)$" % getRootLoggerName(), re.M)
log_re = re.compile("^%s :: BOOM( \(at .*:[0-9]+ in [a-z_]+\))?$" % getRootLoggerName(), re.M)
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure how happy I am with this, since this basically indicates that the location where errors occur are no longer available...

@JensTimmerman: maybe you've been a little bit too eager with putting the if __debug__s in place?

I'll try and see if this can be restored by rolling back one of the if __debug__s without big impact on performance...

Copy link
Contributor

Choose a reason for hiding this comment

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

No I have not been to eager, the location is only visible in debug mode.
if you're not debugging then the location is of no interest to you.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2063/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2065/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Sep 18, 2015

Needs more work:

$ eb --test
== Running framework unit tests...

Running tests...
----------------------------------------------------------------------
.......................F...............................(skipping SvnRepository test)
.................................................................................................................................................................................................................................................................GC3Pie not available, skipping test
...........
======================================================================
ERROR [0.326s]: test_generate_cmd_line (test.framework.options.CommandLineOptionsTest)
Test for generate_cmd_line.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kehoste/work/easybuild-framework/test/framework/options.py", line 1673, in test_generate_cmd_line
    self.assertEqual(ebopts.generate_cmd_line(), [])
AssertionError: Lists differ: ['--test'] != []

First list contains 1 additional elements.
First extra element 0:
--test

- ['--test']
+ []

----------------------------------------------------------------------
Ran 323 tests in 544.241s

FAILED (errors=1)

Generating XML reports...
ERROR: Not all tests were successful.
Log available at /var/folders/8s/_frgh9sj6m744mxt5w5lyztr0000gn/T/eb-59xoyb/eb-F68gCF/eb-9Vdxio/easybuild-tests-PSz63l.log , XML output of tests available in test-reports directory
ERROR: One or more tests failed!

@boegel boegel changed the title use 'python -O' in 'eb' command use 'python -O' in 'eb' command (WIP) Sep 18, 2015
@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2066/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2067/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2068/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2069/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Sep 19, 2015

Jenkins: test this please

@hpcugentbot
Copy link

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2070/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2268/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@wpoely86
Copy link
Member

wpoely86 commented Nov 6, 2015

The -O is fine by me. But running the unit tests from the main program seems a bit weird to me? That's something that's usually clearly separated?

@JensTimmerman
Copy link
Contributor

agreed, unit tests are not usually shipped with the packaged version of an application, and it's not something you would run a lot, only when developing, so I don't see a reason to add it, but now it's there, oh well.
However, --test is probably a bad option to specify, because to me this would imply you are going to test the build of the easyconfig you've given as argument.

the --test option doesn't seem to be an option at all, but more of an alternate argument?
so it becomes clear tha teb test has nothing to do with the options you give when you run
eb easyconfig.eb
and since an easyconfing can actually be called test already, maybe it would be less confusing and clear if you just ad a new eb-test command?

@boegel boegel modified the milestones: v2.5.0, v2.6.0 Dec 14, 2015
@boegel boegel modified the milestones: v2.6.0, v2.7.0 Jan 22, 2016
@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2621/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2623/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2624/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Feb 6, 2016

I removed the --test bit, since it wasn't allowing to run single tests, which is actually what I'm after...

@wpoely86: please (re)review?

This makes a major difference in terms of speed, I really want this merged ASAP.

@wpoely86
Copy link
Member

wpoely86 commented Feb 6, 2016

This all looks fairly trivial?

@boegel
Copy link
Member Author

boegel commented Feb 6, 2016

@wpoely86: yep, the only change needed to make the tests happy is the thing in build_log.

@wpoely86
Copy link
Member

wpoely86 commented Feb 6, 2016

lgtm

@boegel
Copy link
Member Author

boegel commented Feb 6, 2016

Thanks for the review @wpoely86!

boegel added a commit that referenced this pull request Feb 6, 2016
use 'python -O' in 'eb' command & add support for running unit tests with eb --test (WIP)
@boegel boegel merged commit 7a9d431 into easybuilders:develop Feb 6, 2016
@boegel boegel deleted the python_O branch February 6, 2016 21:00
@boegel boegel changed the title use 'python -O' in 'eb' command & add support for running unit tests with eb --test (WIP) use 'python -O' in 'eb' command (REVIEW) Mar 14, 2016
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