Skip to content

Conversation

@jgphpc
Copy link
Contributor

@jgphpc jgphpc commented Sep 10, 2020

Will fix #1476

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #1480 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1480   +/-   ##
=======================================
  Coverage   91.72%   91.72%           
=======================================
  Files          82       82           
  Lines       12924    12924           
=======================================
  Hits        11854    11854           
  Misses       1070     1070           

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 774134e...4331389. Read the comment docs.

@vkarak vkarak self-requested a review September 10, 2020 14:11
@vkarak vkarak added this to the ReFrame sprint 20.13 milestone Sep 10, 2020
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.

One thing that's not very nice is that the configuration option must be enabled in order for the --report-file command-line option to have an effect, which is a bit strange. As a user I would expect to get a report if I pass that option. One solution could be to automatically enable reporting if this option is passed. The downside of this is that it will break somehow the consistency of command line options mapping to configuration options. Another decision to make is what should be the default behaviour: generate or not the report. I kind of tend to not generate, but I'm not sure. This decision will also guide us to what would be the best solution to the problem I just described.

Apart from this, we will need to update the documentation.

@jgphpc
Copy link
Contributor Author

jgphpc commented Sep 15, 2020

Tested with:

  • pytest-3 unittests/test_cli.py
  • reframe hello1.py -> ~/.reframe/reports/run-report.json
  • reframe --report-file=/tmp/0_{sessionid}.json -r -c hello1.py -> /tmp/0_0.json
  • reframe --mode maintenance -C cscs.py -r -c hello1.py -> $APPS/.../run-report-0.json
  • RFM_REPORT_FILE=/tmp/0.json reframe -r -c hello1.py -> /tmp/0.json

@vkarak vkarak changed the title [feat] Add option to disable JSON report [feat] Change default JSON report fill pattern Sep 18, 2020
@vkarak vkarak changed the title [feat] Change default JSON report fill pattern [feat] Change default JSON report file pattern Sep 18, 2020
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.

lgtm now

@vkarak vkarak merged commit 7c1f771 into reframe-hpc:master Sep 18, 2020
@jgphpc jgphpc deleted the json_report branch October 7, 2020 14:11
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.

Add option to disable JSON reporting completely

3 participants