Filter out tests with no programming environments #319

teojgo · 2018-06-07T09:35:32Z

Create the corresponding unit test.

Fixes #244

* Create the corresponding unit test.

codecov-io · 2018-06-07T09:44:25Z

Codecov Report

Merging #319 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   91.18%   91.26%   +0.08%     
==========================================
  Files          67       67              
  Lines        7941     8005      +64     
==========================================
+ Hits         7241     7306      +65     
+ Misses        700      699       -1

Impacted Files	Coverage Δ
unittests/test_utility.py	`98.77% <100%> (+0.03%)`	⬆️
reframe/utility/__init__.py	`93.23% <100%> (+0.67%)`	⬆️
unittests/test_cli.py	`96.19% <100%> (+0.38%)`	⬆️
reframe/utility/sanity.py	`98.76% <100%> (+0.01%)`	⬆️
reframe/frontend/cli.py	`80.41% <100%> (+0.84%)`	⬆️
unittests/test_sanity_functions.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96d3a62...0d14066. Read the comment docs.

vkarak · 2018-06-11T15:30:46Z

reframe/frontend/cli.py

-                                   for e in options.prgenv) else None,
+                lambda c: c if (c.valid_prog_environs
+                                and all(c.supports_environ(e)
+                                        for e in options.prgenv)) else None,


This lambda gets a bit too complicated. Since this operation is quite useful, I suggest writing a function in utility doing that thing:

def allx(iterable): # we should check here specifically if `iterable` is actually an iterable, as real all() does if not iterable: return False return all(iterable)

Would it also make sense to provide a deferrable version of this function in this PR, too?

* Create `allx` function in `reframe/utility/__init__.py`. * Create the corresponding deferrable version of `allx`. * Create the unit tests for the above functions. * Use raw strings in some regular expressions to avoid warnings.

vkarak · 2018-06-12T08:44:13Z

reframe/utility/sanity.py


+@deferrable
+def allx(iterable):
+    """Replacement for the :func:`allx() <reframe.utility.allx>` function.


The documentation of reframe.utility is not public, so you may not refer to it here.

vkarak · 2018-06-12T08:59:26Z

reframe/utility/__init__.py


+def allx(iterable):
+    """Return ``True`` if all elements of the `iterable` are true. If
+    iterable is empty or ``None`` return ``False``.


I would phrase this different to stress the connection with the builtin all(): Same as the built-in all, except that it returns False if iterable is empty.

This function must be fully equivalent with the built-in all() if iterable is not empty, which means, we should raise TypeError if the argument is None. Pay attention to have fully compatible behaviour.

There are several documentation formatting issues here:

Double backquotes must be used for citing function arguments, here iterable.

None, True and False must be formatted as :class:`None` , :class:`True` etc.

vkarak · 2018-06-12T09:28:33Z

reframe/utility/__init__.py

+            first_item = next(iterable)
+            return all(itertools.chain([first_item], iterable))
+        except StopIteration:
+            return False


I can't understand all this fuss here. Why a check of iterable against collections.abc.Iterable wouldn't just work?

>>> import collections >>> isinstance(range(0), collections.abc.Iterable) True >>>

Because we do not want to accept empty generators. Is there any other way to check that a generator is empty?

What do you mean by "empty generator"? A generator is a generator, i.e., an iterable, even if it doesn't produce any values. Your unit tests should pass fine using my proposal.

I mean a generator like (i for i in []) if it is passed to allx how can I check that there are no elements and return False?

Now, I get what you are trying to do. The comment above the code is really misleading... Please change it with sth more precise!

vkarak · 2018-06-12T09:29:15Z

reframe/utility/__init__.py

+        except StopIteration:
+            return False
+
+    if not iterable:


This is not enough. If iterable is None, we should raise a TypeError as all() does.

vkarak · 2018-06-12T11:01:04Z

unittests/test_sanity_functions.py

+        self.assertFalse(expr3)
+        self.assertFalse(expr4)
+        self.assertFalse(expr5)
+        self.assertTrue(expr6)


You can combine these assertions with the expressions above.

vkarak · 2018-06-12T11:25:26Z

unittests/test_sanity_functions.py

+        expr3 = sn.allx([])
+        expr4 = sn.allx(None)
+        expr5 = sn.allx(i for i in [])
+        expr6 = sn.allx(i for i in [1])


I would use ranges here.

vkarak · 2018-06-12T11:43:10Z

reframe/utility/__init__.py

+    if isinstance(iterable, types.GeneratorType):
+        try:
+            first_item = next(iterable)
+            return all(itertools.chain([first_item], iterable))


Better put this in the else block of the try/except. Also rename first_item to first or head.

- Rationale on special treatment of generators in allx. - Document the sanity function correctly. - Correct line splits.

…-fixes Code organisation fixes

teojgo · 2018-06-14T06:44:05Z

@jenkins-cscs retry all

vkarak

@teojgo @victorusu I think there is a fundamental problem with this PR:

On master, this

./reframe.py -C config/cscs.py -c cscs-checks/ -R -l

gives a whole listing of the tests, whereas here it gives nothing!

teojgo · 2018-06-14T12:40:25Z

@victorusu what should be the expected behavior when a check has an empty prgenv list and we also do not specify any particular prgenv to ReFrame?

vkarak · 2018-06-14T13:15:48Z

@teojgo @victorusu The logical thing to do would be to print the test if it supports any programming environment, i.e., valid_prog_environs is not empty.

vkarak · 2018-06-14T14:57:32Z

reframe/frontend/cli.py

+                # Here we also check that the valid_prog_environs of the check
+                # is an iterable and not empty
+                filter_func = lambda c: c if util.allx(
+                    c.valid_prog_environs) else None


I suggest replacing the lambdas altogether here in favour of a closure:

def filter_prgenv(c): if options.prgenv: return util.allx(c.supports_environ(e) for e in options.prgenv) else: return bool(c.valid_prog_environs)

Add extra filter for empty prg environs

8d47856

* Create the corresponding unit test.

teojgo added request for enhancement prio: normal frontend labels Jun 7, 2018

teojgo self-assigned this Jun 7, 2018

teojgo requested review from victorusu and vkarak June 7, 2018 09:35

vkarak requested changes Jun 11, 2018

View reviewed changes

Address PR comments

771c641

* Create `allx` function in `reframe/utility/__init__.py`. * Create the corresponding deferrable version of `allx`. * Create the unit tests for the above functions. * Use raw strings in some regular expressions to avoid warnings.

vkarak requested changes Jun 12, 2018

View reviewed changes

vkarak reviewed Jun 12, 2018

View reviewed changes

Theofilos Manitaras and others added 3 commits June 12, 2018 13:55

Addres PR comments (version 2)

142f89a

Code organisation fixes

4f4377c

- Rationale on special treatment of generators in allx. - Document the sanity function correctly. - Correct line splits.

Merge pull request #326 from vkarak/enhancement/exclude_empty_prgenvs…

e0c343b

…-fixes Code organisation fixes

Merge branch 'master' into enhancement/exclude_empty_prgenvs

b817683

vkarak approved these changes Jun 14, 2018

View reviewed changes

vkarak requested changes Jun 14, 2018

View reviewed changes

Check for empty 'options.prgenv'

e1e5a43

vkarak requested changes Jun 14, 2018

View reviewed changes

Address PR comments (version 3)

eb34e05

vkarak approved these changes Jun 15, 2018

View reviewed changes

vkarak changed the title ~~Add extra filter for empty prg environs~~ Filter out tests with no programming environments Jun 15, 2018

Merge branch 'master' into enhancement/exclude_empty_prgenvs

0d14066

vkarak merged commit f87f557 into master Jun 15, 2018

vkarak deleted the enhancement/exclude_empty_prgenvs branch June 15, 2018 10:58

Filter out tests with no programming environments #319

Filter out tests with no programming environments #319

Uh oh!

Conversation

teojgo commented Jun 7, 2018

Uh oh!

codecov-io commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

vkarak Jun 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

teojgo commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkarak left a comment

Choose a reason for hiding this comment

Uh oh!

teojgo commented Jun 14, 2018

Uh oh!

vkarak commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jun 7, 2018 •

edited

Loading

vkarak Jun 12, 2018 •

edited

Loading

teojgo commented Jun 14, 2018 •

edited

Loading