Skip to content

Conversation

@rsarm
Copy link
Contributor

@rsarm rsarm commented Oct 11, 2018

Closes #473

@rsarm rsarm added this to the ReFrame sprint 2018w41 milestone Oct 11, 2018
@rsarm rsarm self-assigned this Oct 11, 2018
@rsarm rsarm requested a review from vkarak October 11, 2018 10:09
@vkarak vkarak requested review from teojgo and victorusu October 11, 2018 11:39
Copy link
Contributor

@teojgo teojgo left a comment

Choose a reason for hiding this comment

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

Change also the parts of the documentation in the Running ReFrame section which demonstrate using the -l option.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

It's nice. I have just a couple of coding suggestions.

(self.name, inspect.getfile(type(self)),
self.descr, ','.join(self.tags), self.maintainers))
return ("%s(name='%s', prefix='%s')" %
(self.__class__.__name__, self.name, self.prefix))
Copy link
Contributor

Choose a reason for hiding this comment

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

Rewrite this as follows:

        return "%s(name='%s', prefix='%s')" % (type(self).__name__, 
                                               self.name, self.prefix)

', '.join(check.tags), ', '.join(check.maintainers)))
else:
return (' * %s (found in %s)' %
(check.name, inspect.getfile(type(check))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Rewrite this as follows:

lines = ['  * %s (found in %s)' % (check.name, inspect.getfile(type(check)))]
if detailed:
    lines += ['...', '...']

return '\n'.join(lines)



def list_checks(checks, printer):
def format_check_description(check, detailed):
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to format_check.

help='List matched regression checks')
action_options.add_argument(
'-L', '--list-detailed', action='store_true',
help='Detailed description of matched regression checks')
Copy link
Contributor

Choose a reason for hiding this comment

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

List matched regression checks with a detailed description

(self.name, inspect.getfile(type(self)),
self.descr, ','.join(self.tags), self.maintainers))
return ("%s(name='%s', prefix='%s')" %
(type(self).__name__, self.name, self.prefix))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's against the coding style to put parentheses around the return value, unless it is absolutely necessary. Can you rewrite this as I proposed before?

@vkarak vkarak changed the title [feat] Add support for more verbose output in tests listing [feat] Add new command-line option for more verbose test listing Oct 12, 2018
@codecov-io
Copy link

codecov-io commented Oct 12, 2018

Codecov Report

Merging #506 into master will increase coverage by 0.02%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
+ Coverage   91.39%   91.42%   +0.02%     
==========================================
  Files          72       72              
  Lines        8749     8767      +18     
==========================================
+ Hits         7996     8015      +19     
+ Misses        753      752       -1
Impacted Files Coverage Δ
reframe/core/pipeline.py 91.7% <0%> (ø) ⬆️
reframe/frontend/cli.py 80.68% <100%> (+1.07%) ⬆️
unittests/test_cli.py 96.02% <100%> (+0.13%) ⬆️

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 8dd86c4...00187ef. Read the comment docs.

@vkarak
Copy link
Contributor

vkarak commented Oct 12, 2018

@rsarm Please also add a simple unit test that calls the -L option, so as to test this code path as well.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

I have some comments regarding the documentation, which I am fixing them right now.

Command line: ./bin/reframe -C tutorial/config/settings.py -c tutorial/advanced/advanced_example8.py -l
Reframe version: 2.13-dev0
Reframe version: 2.15-dev0
Copy link
Contributor

Choose a reason for hiding this comment

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

The version here should be 2.15-dev1

docs/running.rst Outdated
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To retrieve a listing of the selected checks, you must specify the ``-l`` or ``--list`` options.
There are two ways to retrieve a listing of the selected checks. One is by specifying the ``-l`` or ``--list`` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

The guideline in the documentation is to put a single sentence per line, so as to facilitate the diffs and avoid conflicts. There is now line limit in the documentation.

docs/running.rst Outdated
.. code-block:: none
Command line: ./bin/reframe -c tutorial/ -L
Reframe version: 2.15-dev0
Copy link
Contributor

Choose a reason for hiding this comment

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

2.15-dev1

docs/running.rst Outdated
Command line: ./reframe.py -C tutorial/config/settings.py -c tutorial/example1.py -r
Reframe version: 2.13-dev0
Reframe version: 2.15-dev0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not change manually the versions numbers if you don't re-reproduce the output. The date just after this output shows May 26, when the 2.15 wasn't available!

docs/running.rst Outdated
Command line: ./bin/reframe -C tutorial/config/settings.py -c tutorial/ --exec-policy=async -r
Reframe version: 2.13-dev0
Reframe version: 2.15-dev0
Copy link
Contributor

Choose a reason for hiding this comment

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

The version should still remain 2.13-dev0 here.

@vkarak vkarak merged commit 0081b23 into reframe-hpc:master Oct 16, 2018
@rsarm rsarm deleted the checks/detailed-listing branch November 26, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants