Add style tests #1618

wpoely86 · 2016-02-17T16:52:55Z

No description provided.

Still fails for the moment.

wpoely86 · 2016-02-17T16:53:12Z

The first Jenkins test should fail.

hpcugentbot · 2016-02-17T17:23:24Z

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2710/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 · 2016-02-17T21:31:04Z

easybuild/framework/easyconfig/style.py

+# definied as AXXX where A is either E or W and XXX is a 3 digit number.
+# It should be mentioned in the docstring as a single word.
+# Read the pep8 docs to understand the arguments of these functions
+def eb_check_trailing_whitespace(physical_line, lines, line_number, total_lines):


this needs @only_if_module_is_available('pep8') too?

well, this should be a 'private' function. Never ever be called outside this module.

sure, but you still need the guard imho (since it uses pep8)

If you want to make it a private function, rename it to _eb_check...

boegel · 2016-02-17T21:36:55Z

@wpoely86: it fails, hooray! Now fix it? ;)

hpcugentbot · 2016-02-18T09:08:57Z

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2718/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 · 2016-02-18T09:20:12Z

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2719/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 · 2016-02-24T22:21:07Z

Jenkins: test this please

hpcugentbot · 2016-02-24T22:58:01Z

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2767/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 · 2016-02-25T11:14:18Z

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2772/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.

@geimer

* develop: (58 commits) fix remark make default is_short_modname_for check less strict to support versionless external modules as deps Update toolchain.py loosen up mod exists regex for show output & clean up better docstrings enhance ModulesTool.exist to also recognize partial module names fix remark Hercules foundation is now FWO fix remark fix fetch_easyconfigs_from_pr w.r.t. duplicate files in PRs add Description string also in lua modulefiles trying to fix unit tests removed space change suggested by @geimer trying to fix unit tests correct fix, I hope :) undo previous change fix bug triggered with use of --stop + add test that triggers it very minor style fix rewrite get_toolchain_hierarchy to fix @ocaisa's remarks ...

hpcugentbot · 2016-02-25T14:01:20Z

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2775/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 · 2016-02-25T20:17:38Z

easybuild/framework/easyconfig/style.py

+except ImportError:
+    pass
+
+_log = fancylogger.getLogger('easyconfig.style', fname=False)


why add a logger if you're not using it?

(don't remove it, use it below :))

boegel · 2016-11-24T08:26:03Z

I need to get back to this, but rather than cooking up our own approach, we can also rely on an external service that integrates with GitHub, like http://docs.sonarqube.org/display/PLUG/Python+Plugin

wpoely86 · 2016-11-24T08:30:29Z

Why? We are replying on pep8 and pylint? I think style is something which we want to control ourself?

boegel · 2016-11-24T08:34:19Z

@wpoely86 as far as I can tell, you can get full control over how you define your code style via SonarQube too, you can supply a custompylintrc, and it integrates well with Travis (see for example apptainer/singularity#341)

And it avoids that we have to write & maintain our own stuff (although we still may have to do that for easyconfigs, I'm mostly talking out the actual Python code in framework/easyblocks here).

wpoely86 · 2016-11-24T08:39:43Z

This PR is only about style testing for easyconfigs?

* develop: (962 commits) change log.debug to log.info for sanity_check_commands run 'python setup.py sdist' from correct directory make tests more robust against running headless run framework test suite on installation from sdist source tarball to catch packaging issues fix packaging issue with non-Python scripts in easybuild/scripts use setvar in modules.py to define environment variables don't specify modules tool/syntax to use, bootstrap should get it right by itself enable debugging of bootstrap script only emit debug message on module syntax if $EASYBUILD_MODULE_SYNTAX is defined use correct module syntax in bootstrap script if Lmod is not used export env vars to be picked up by bootstrap script re-enable 'eb --version' check after bootstrap export env vars to be picked up by bootstrap script bump version to 3.1.0.dev0 fix remark in v3.0.0 release notes bump version to 3.0.0, update release notes bump version of bootstrap script (missing in easybuilders#1984) bump Lmod test version to 6.6.3 since 6.6.2 was removed, also test against Lmod 7.0 bump Lmod test version to 6.6.3 since 6.6.2 was removed include tmpdir in RPATH filter via tempfile.gettempdir() ...

boegel · 2016-11-29T12:52:48Z

test/framework/style.py

+            #    self.assertTrue(False, "pep8 not available")
+            # else:
+            #    print "Skipping style checks (no pep8 available)"
+            #    return


@wpoely86 please remove this

boegel · 2016-11-29T12:53:50Z

test/framework/style.py

+
+def suite():
+    """Return all style tests for easyconfigs."""
+    return TestLoader().loadTestsFromTestCase(StyleTest)


@wpoely86 please use return TestLoaderFiltered().loadTestsFromTestCase(StyleTest, sys.argv[1:]) to allow filtering of tests

boegel · 2016-11-29T12:54:55Z

test/framework/style.py

+
+
+if __name__ == '__main__':
+    main()


switch to TextTestRunner(verbosity=1).run(suite()), see #1828

boegel · 2016-11-29T12:55:57Z

test/framework/style.py

+            #    return
+
+        # all available easyconfig files
+        test_easyconfigs_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs')


test easyconfigs are in a different place now, see #1970

boegel · 2016-11-29T13:38:10Z

.travis.yml

    - if [ "x$TRAVIS_PYTHON_VERSION" == 'x2.6' ]; then pip install pydot==1.1.0; else pip install pydot; fi
    # optional Python packages for EasyBuild
-    - pip install autopep8 GC3Pie python-graph-dot python-hglib PyYAML
+    - pip install pep8 autopep8 GC3Pie python-graph-dot python-hglib PyYAML


style nitpicking: retain alphabetical order

boegel · 2016-11-29T13:41:22Z

easybuild/framework/easyconfig/style.py

+
+
+@only_if_module_is_available('pep8')
+def style_conformance(easyconfigs, verbose=False):


rename to check_easyconfigs_style ?

boegel · 2016-11-29T13:41:47Z

easybuild/framework/easyconfig/style.py

+    """
+    Check the given list of easyconfigs for style
+    :param easyconfigs list of file paths to easyconfigs
+    :param verbose print our statistics and be verbose about the errors and warning


missing : after name of arguments

boegel · 2016-11-29T13:41:55Z

easybuild/framework/easyconfig/style.py

+    Check the given list of easyconfigs for style
+    :param easyconfigs list of file paths to easyconfigs
+    :param verbose print our statistics and be verbose about the errors and warning
+    :return the number of warnings and errors


missing :

boegel · 2016-11-29T13:45:10Z

easybuild/framework/easyconfig/style.py

+    options.ignore = (
+        'E402',  # import not on top
+        'W291',  # replaced by W299
+        'E501',  # line too long


Why are we ignoring E402 and E501? If there's a good reason, please mention why.

E402 because some easyconfigs imported stuff (not on top)
E501 because some easyconfigs have very long lines

both of them are a valid reason to fail the style check?

boegel · 2016-11-29T14:24:51Z

test/framework/easyconfigs/test_ecs/g/gzip/gzip-1.6-GCC-4.9.2.eb


 name = 'gzip'
-version = '1.6'  # FIXME bug: quotes are required here to make sure this is parsed a a string, not a floating point value
+# FIXME bug: quotes are required here to make sure this is parsed a a string, not a floating point value


@wpoely86 please drop this comment, it doesn't make any sense here (it's copy-pasted from the equivalent .yeb easyconfig)

boegel · 2016-11-29T14:26:43Z

test/framework/style.py

+            return
+
+        # all available easyconfig files
+        test_easyconfigs_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs', 'test_ecs')


@wpoely86 based on the tweaked test easyconfigs, we should probably revert testing only the easyconfigs in test_ecs, and test all test easyconfigs (including the .yeb ones)? I missed that earlier, sorry...

oh wait, .yeb easyconfigs aren't going to pass a PEP8 test... :)

so, leave as is

boegel · 2016-11-30T13:56:09Z

lgtm, thanks for all the effort on this @wpoely86!

wpoely86 · 2016-11-30T15:33:11Z

Easyconfig use is in easybuilders/easybuild-easyconfigs#2506

wpoely86 added 3 commits February 17, 2016 17:35

Add style check for easyconfigs

06a5646

Rename function

dd95739

Add test for style

276f17a

Still fails for the moment.

wpoely86 mentioned this pull request Feb 17, 2016

enable automatic style checks in easyconfig tests easybuilders/easybuild-easyconfigs#2506

Merged

boegel added this to the v2.7.0 milestone Feb 17, 2016

boegel reviewed Feb 17, 2016
View reviewed changes

wpoely86 added 3 commits February 18, 2016 09:42

Update test easyconfig to pass style check

9e92dab

Fix remarks

d482ff2

Make function private

adac638

Fix checksums for tests

9056d23

wpoely86 added 2 commits February 25, 2016 14:05

Fix style

6e62431

boegel reviewed Feb 25, 2016
View reviewed changes

boegel removed this from the v3.0 milestone Nov 10, 2016

wpoely86 added 4 commits November 24, 2016 11:13

Fix author tag

210cc17

Add pep8 to travis.yml

5a51dee

Some style changes

e1bf239

boegel reviewed Nov 29, 2016

View reviewed changes

test/framework/style.py Outdated

if __name__ == '__main__':

main()

Copy link

Member

boegel Nov 29, 2016

Choose a reason for hiding this comment

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

switch to TextTestRunner(verbosity=1).run(suite()), see #1828

boegel reviewed Nov 29, 2016

View reviewed changes

Fix remarks

854f1dc

boegel reviewed Nov 29, 2016

View reviewed changes

Fix imports and order

5e047ca

boegel reviewed Nov 29, 2016

View reviewed changes

wpoely86 added 2 commits November 29, 2016 14:52

Fix more remarks

d92bb09

Use constants and refactor

a12c72c

boegel requested changes Nov 29, 2016

View reviewed changes

Drop unneeded comment

795048f

boegel approved these changes Nov 29, 2016

View reviewed changes

boegel merged commit 0348836 into easybuilders:develop Nov 30, 2016

wpoely86 deleted the style branch November 30, 2016 13:56

boegel mentioned this pull request Jan 14, 2017

add first version of script to check code style in easyconfigs (WIP) #1555

Closed



		@only_if_module_is_available('pep8')
		def style_conformance(easyconfigs, verbose=False):

Add style tests #1618

Add style tests #1618

Uh oh!

Conversation

wpoely86 commented Feb 17, 2016

Uh oh!

wpoely86 commented Feb 17, 2016

Uh oh!

hpcugentbot commented Feb 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Feb 17, 2016

Uh oh!

hpcugentbot commented Feb 18, 2016

Uh oh!

hpcugentbot commented Feb 18, 2016

Uh oh!

boegel commented Feb 24, 2016

Uh oh!

hpcugentbot commented Feb 24, 2016

Uh oh!

hpcugentbot commented Feb 25, 2016

Uh oh!

hpcugentbot commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Nov 24, 2016

Uh oh!

wpoely86 commented Nov 24, 2016

Uh oh!

boegel commented Nov 24, 2016

Uh oh!

wpoely86 commented Nov 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Nov 30, 2016

Uh oh!

wpoely86 commented Nov 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

boegel Nov 29, 2016 •

edited

Loading