update computer test output #3544

ltalirz · 2019-11-14T13:14:00Z

This PR updates the output of verdi computer test

Old output:

New output:

ltalirz · 2019-11-14T13:16:57Z

aiida/schedulers/plugins/test_slurm.py

+
+        self.assertEquals(get_job('861352').title, 'Pressure_PBEsol_0')
+
+        self.assertEquals(get_job('863554').requested_wallclock_time_seconds, None)  # not set


One thing I noticed is that this time will also be None, if the value could not be parsed.
Perhaps ok, but it means that finding a value None does no longer mean that parsing failed.

sphuber · 2019-11-27T19:36:17Z

I feel the change you are making to the scheduler parsing is too important to be muffled away in a commit on changing the output format of a computer test function. Would you be willing to separate that in another PR?

ltalirz · 2019-11-27T21:33:51Z

Would you be willing to separate that in another PR?

Done #3586

sphuber

Some minor visual suggestions, but ok to leave it as is if you disagree

sphuber · 2019-11-27T21:40:11Z

aiida/cmdline/commands/cmd_computer.py

@@ -141,11 +142,11 @@ def _computer_create_temp_file(transport, scheduler, authinfo):  # pylint: disab

    file_content = "Test from 'verdi computer test' on {}".format(datetime.datetime.now().isoformat())
    echo.echo('> Creating a temporary file in the work directory...')
-    echo.echo('  `-> Getting the remote user name...')
+    echo.echo('  -> Getting the remote user name...')


Maybe it is nice to put the [OK] on the same line as the first line.
This would go for all other lines as well where there is no other intermediate result but just an ok.
Put the message with ellipses and OK on the same line

I agree this looks nicer and I wanted to do this at first. It's nasty though - what do you do if the test fails?

During test failures, some commands print random stuff, which will then inevitably land on the same line.
For the "echo" statements originating from the command itself one could handle it, but also there you would need to do quite a bit of if/else in order to avoid having stuff printed on the same line that shouldn't be.

I've made the changes here https://github.com/ltalirz/aiida-core/tree/computer-test-echo-nl-false
Let me know what you prefer.

If it fails, we can still print the error after a [FAILED] right? Or even on the next line if it is multiple lines. But I would imagine something like this:

* Getting job list... [OK]: Found 6 jobs in the queue * Checking that no spurious output is present... [FAILED] * Getting the remote user name... [OK]: <ltalirz> * Retrieving the file and checking its content... [FAILED]: file not found

well, that is if you control everything that happens between printing "..." and the failure.
currently, many of the methods being called here just randomly print stuff (and not just the ones defined inside this file).
I'd have to start capturing all of this - can be done but is quite some work

we can just put the calling blocks in with Capturing() no? If you are ok with it, I will gladly update your other branch to do it.

sure, please go ahead!
you have write acces to my fork already

sphuber · 2019-11-27T21:40:57Z

aiida/cmdline/commands/cmd_computer.py


    handle, destfile = tempfile.mkstemp()
    os.close(handle)
    try:
        transport.getfile(remote_file_path, destfile)
        with io.open(destfile, encoding='utf8') as dfile:
            read_string = dfile.read()
-        echo.echo('      [Retrieved]')
+        echo.echo('     [Retrieved]')


maybe remove this one and just put [OK] instead of [Content OK] on same line.

sphuber · 2019-11-27T21:41:12Z

aiida/cmdline/commands/cmd_computer.py

    transport.remove(remote_file_path)
-    echo.echo('  [Deleted successfully]')
+    echo.echo('     [Deleted successfully]')


same here, just [OK] would suffice?

sphuber · 2019-11-27T21:41:37Z

aiida/cmdline/commands/cmd_computer.py

        else:
-            echo.echo('Test completed (all {} tests succeeded)'.format(num_tests))
+            echo.echo_success('All {} tests succeeded)'.format(num_tests))


the string contains unbalanced closing parens

sphuber · 2019-11-28T12:13:14Z

@ltalirz I have updated the branch. I intended to push this to your second "test" branch but accidentally did it to the PR branch. Anyway, I didn't touch your commit, just rebased it so it is up-to-date. The additional commit will make it look as follows:

Success

Failed

With traceback

ltalirz · 2019-11-28T12:42:07Z

aiida/cmdline/commands/cmd_computer.py

    try:
-        echo.echo('> Testing connection...')
+        echo.echo('* Opening connection... ', nl=False)
+
        with transport:


What if an error happens here (e.g. configure a wrong username).
It will start printing stuff...

I don't think so, it will be caught in the outer try-except and that also just prints the error or including the traceback if the option is specified. Example:

ltalirz · 2019-11-28T12:43:09Z

aiida/cmdline/commands/cmd_computer.py

            num_tests += 1
-            for test in [_computer_test_no_unexpected_output, _computer_test_get_jobs, _computer_create_temp_file]:
+
+            click.secho('[OK]', fg=echo.COLORS['success'], bold=echo.BOLD)


in my 2nd PR I introduced an echo.echo_highlight command that saves one from specifying the bold parameter + one can pass directly 'success'.
Can save some text...

I have added this to your first commit and then used it in mine.

ltalirz

For me it's anyhow an improvement and good to merge.

@sphuber Since it's my PR, you will need to approve yourself ;-)

* fix indentation * add color to results of all 3 tests * add "success" to final message * add `aiida.cmdline.utils.echo.echo_highlight` function

The number of tests is increased and messaging from within each test is disabled. The tests now return a tuple of a boolean indicating success or failure and an optional message. This message can be informational in case of success or an error message. With echo'ing only being performed in the main function, the output can be given a consistent look. Each test will have `[OK]` or `[FAILED]` on the same line with an optional message. Additional lines, such as for the full traceback, are prepended with newline and two spaces of indentation.

sphuber · 2019-11-28T13:36:17Z

For me it's anyhow an improvement and good to merge.

@sphuber Since it's my PR, you will need to approve yourself ;-)

I included echo_highlight to your commit and then used it in mine. If you can confirm once more that you are happy with this I will merge it. Thanks

ltalirz

Thanks @sphuber , looks good to me!

sphuber

Thanks @ltalirz

ltalirz changed the title ~~Computer test echo~~ update computer test output Nov 14, 2019

ltalirz commented Nov 14, 2019

View reviewed changes

ltalirz requested a review from sphuber November 27, 2019 19:24

ltalirz force-pushed the computer-test-echo branch from edcabfa to 7ff62a8 Compare November 27, 2019 21:33

sphuber reviewed Nov 27, 2019

View reviewed changes

sphuber force-pushed the computer-test-echo branch from 7ff62a8 to 9007485 Compare November 28, 2019 12:11

ltalirz commented Nov 28, 2019

View reviewed changes

ltalirz and others added 2 commits November 28, 2019 14:29

Homogenize output of verdi computer test

33068c2

* fix indentation * add color to results of all 3 tests * add "success" to final message * add `aiida.cmdline.utils.echo.echo_highlight` function

sphuber force-pushed the computer-test-echo branch from 9007485 to 09fb35b Compare November 28, 2019 13:35

ltalirz commented Nov 28, 2019

View reviewed changes

sphuber self-requested a review November 28, 2019 14:18

sphuber approved these changes Nov 28, 2019

View reviewed changes

sphuber merged commit e6430f7 into aiidateam:develop Nov 28, 2019

sphuber deleted the computer-test-echo branch November 28, 2019 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update computer test output #3544

update computer test output #3544

ltalirz commented Nov 14, 2019 •

edited

Loading

ltalirz Nov 14, 2019

sphuber commented Nov 27, 2019

ltalirz commented Nov 27, 2019

sphuber left a comment

sphuber Nov 27, 2019

ltalirz Nov 27, 2019

sphuber Nov 28, 2019

ltalirz Nov 28, 2019

sphuber Nov 28, 2019

ltalirz Nov 28, 2019

sphuber Nov 27, 2019

sphuber Nov 27, 2019

sphuber Nov 27, 2019

sphuber commented Nov 28, 2019 •

edited

Loading

ltalirz Nov 28, 2019

sphuber Nov 28, 2019 •

edited

Loading

ltalirz Nov 28, 2019

sphuber Nov 28, 2019

ltalirz left a comment

sphuber commented Nov 28, 2019

ltalirz left a comment

sphuber left a comment


		self.assertEquals(get_job('861352').title, 'Pressure_PBEsol_0')

		self.assertEquals(get_job('863554').requested_wallclock_time_seconds, None) # not set

update computer test output #3544

update computer test output #3544

Conversation

ltalirz commented Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

sphuber commented Nov 27, 2019

ltalirz commented Nov 27, 2019

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Nov 28, 2019 • edited Loading

Success

Failed

With traceback

Choose a reason for hiding this comment

sphuber Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz left a comment

Choose a reason for hiding this comment

sphuber commented Nov 28, 2019

ltalirz left a comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

ltalirz commented Nov 14, 2019 •

edited

Loading

sphuber commented Nov 28, 2019 •

edited

Loading

sphuber Nov 28, 2019 •

edited

Loading