Improve automatic test suite's HTML reports #2903

russkev · 2020-11-24T11:03:17Z

Addresses issue #2853 . Git hash and title now added to HTML report.

Addresses issue #2844. I replicated the the statistics that get displayed in the console in the HTML report. I originally intended the info to be at the top of the page but that needed a lot more work than putting it at the bottom, so I went with that instead.

oktomus

This is awesome ! It's been a long time since we want this on our test report 😄

Thanks a lot :)

oktomus · 2020-11-29T12:49:47Z

scripts/runtestsuite/runtestsuite.py


        self.file.write(self.__render(self.header_template,
                                      {'test-date': CURRENT_TIME,
                                       'python-version': utils.get_python_version(),
                                       'script-path': script_path,
-                                       'script-version': VERSION,
+                                       'script-version': "HELLO WORLD", #VERSION,


Hello ! 👋

oktomus · 2020-11-29T12:50:57Z

scripts/runtestsuite/runtestsuite.py

+        git_hash =  subprocess.check_output(['git', 'rev-parse', 'HEAD'])
+        git_title = subprocess.check_output(['git', 'rev-parse', '--abbrev-ref', 'HEAD'])


What happens if git is not installed or the appleseed version used is a release (we remove .git from the release) ?

Ah, good call, I'll add some checks.

oktomus · 2020-11-29T12:51:53Z

scripts/runtestsuite/runtestsuite.py

+
+    def __write_stats(self, total_time, success, failures):
+        success_rate = "{0}%".format(success)
+        failures_str = "{0} out of {1} test scene(s)".format(failures[0], failures[1])


Minor styling issue : here you use the prefix _str, but not everywhere else. You can remove that prefix to have a more consistent code base :)

oktomus · 2020-11-29T12:53:28Z

scripts/runtestsuite/runtestsuite.py

        self.file.flush()
+
+    def __write_stats(self, total_time, success, failures):


By reading the argument names, I understand that success and failures are both of the same type (a rate or an amount). But when reading the code bellow, I realized that success was a rate and failures an amount. Do you think you can change the naming ot the value themeselves to have something more consistent ?

oktomus · 2020-11-29T12:53:58Z

scripts/runtestsuite/runtestsuite.py

+        git_hash =  subprocess.check_output(['git', 'rev-parse', 'HEAD'])
+        git_title = subprocess.check_output(['git', 'rev-parse', '--abbrev-ref', 'HEAD'])


Maybe you could create methods for those, as you already did for get_git_revision_hash

scripts/runtestsuite/runtestsuite.py

scripts/runtestsuite/header_template.html

Co-authored-by: Kevin Masson <hi@oktomus.com>

- Check scripts directory for git information instead of calling directory - Check that git is installed - Check that scripts directory is under git control - Variables renamed for clarity - Refactored various methods

russkev · 2020-12-01T05:50:23Z

Thanks for your feedback, @oktomus. I've attempted to address most of it in my latest commit.

oktomus

Great work ! My last comments are mostly about style, to make sure it matches with the existing code base.

oktomus · 2020-12-01T11:54:31Z

scripts/runtestsuite/runtestsuite.py

+def git_installed(directory):
+    temp = 'git -C {0} --version'.format(directory)
+    print(temp)
+    return command_is_valid('git -C {0} --version'.format(directory))
+
+def under_git_control(directory):
+    return command_is_valid('git -C {0} rev-parse --is-inside-work-tree'.format(directory))


Suggested change

def git_installed(directory):

temp = 'git -C {0} --version'.format(directory)

print(temp)

return command_is_valid('git -C {0} --version'.format(directory))

def under_git_control(directory):

return command_is_valid('git -C {0} rev-parse --is-inside-work-tree'.format(directory))

def is_git_installed(directory):

return command_is_valid('git -C {0} --version'.format(directory))

def is_git_repository(directory):

return command_is_valid('git -C {0} rev-parse --is-inside-work-tree'.format(directory))

Here is a suggestion that remove the unused var and change the naming

oktomus · 2020-12-01T11:55:02Z

scripts/runtestsuite/runtestsuite.py

+    return command_output('git -C {0} rev-parse --abbrev-ref HEAD'.format(directory))
+
+def command_output(command):
+    output = "N/A"


Suggested change

output = "N/A"

output = None

Isn't this more consistent with Python conventions ?

This is what will be printed when the command fails (e.g. get_git_hash fails). I thought it would be more consistent to have it print N\A rather than none.

I could have an if else statement inside __write_header but I thought that would make it unnecessarily messy, do you think it would be a better choice?

I think it's a good idea to print "N/A" for the user. But here the method is command_output and we don't expect it to improve the output by default.

oktomus · 2020-12-01T11:57:50Z

scripts/runtestsuite/runtestsuite.py

+    is_valid = 1
+    try:
+        is_valid = subprocess.check_call(
+            command.split(),
+            stdin = subprocess.PIPE, 
+            stdout = open(os.devnull, 'wb'),
+            stderr = subprocess.STDOUT)
+    except (subprocess.CalledProcessError, OSError):
+        pass
+
+    return is_valid == 0


Here is an alternative that doesn't require an int and is easier to understand

Suggested change

is_valid = 1

try:

is_valid = subprocess.check_call(

command.split(),

stdin = subprocess.PIPE,

stdout = open(os.devnull, 'wb'),

stderr = subprocess.STDOUT)

except (subprocess.CalledProcessError, OSError):

pass

return is_valid == 0

try:

command_output = subprocess.check_call(

command.split(),

stdin = subprocess.PIPE,

stdout = open(os.devnull, 'wb'),

stderr = subprocess.STDOUT)

return command_output == 0

except (subprocess.CalledProcessError, OSError):

pass

return False

Althouth, I don't understand why calling .split() on command os required, and why you need to specify the standard output and error 🤔

As far as I can tell, subprocess doesn't read spaces properly. Anything separated by a space, needs to be separate elements of an array.

I'm specifying the standard output and error so they don't get displayed in the terminal (and disrupt the nice table that gets printed). It is possible that this would be cleaner if I used another subprocess method here, I'll have to investigate.

Ok, good to know ! We can keep this, but maybe you can mention these important details directly in the code ?

oktomus · 2020-12-01T12:01:39Z

scripts/runtestsuite/runtestsuite.py

+        git_hash = git_title = "N/A"
+
+        if git_installed(self.template_directory):
+            if under_git_control(self.template_directory):
+                git_hash = get_git_hash(self.template_directory)
+                git_title = get_git_title(self.template_directory)
+            else:
+                git_hash = git_title = "N/A (Directory not under git control)"
+        else:
+            git_hash = git_title = "N/A (Git not installed)"


Do you think this is easier to understand ?

Suggested change

git_hash = git_title = "N/A"

if git_installed(self.template_directory):

if under_git_control(self.template_directory):

git_hash = get_git_hash(self.template_directory)

git_title = get_git_title(self.template_directory)

else:

git_hash = git_title = "N/A (Directory not under git control)"

else:

git_hash = git_title = "N/A (Git not installed)"

is_git_installed = git_installed(self.template_directory)

is_git_repository = under_git_control(self.template_directory)

git_hash = git_title = None

if is_git_installed and is_git_repository:

git_hash = get_git_hash(self.template_directory)

git_title = get_git_title(self.template_directory)

elif not is_git_installed:

git_hash = git_title = "N/A (Git not installed)"

else:

git_hash = git_title = "N/A (Directory not under git control)"

oktomus · 2020-12-01T12:06:18Z

scripts/runtestsuite/runtestsuite.py

+    def __write_stats(self, total_time, success_rate, failures, rendered_scenes):
+        success_rate = "{0}%".format(success_rate)
+        failures = "{0} out of {1} test scene(s)".format(failures, rendered_scenes)
+        self.file.write(self.__render(self.stats_template,
+                                       {'total-time': format_duration(total_time),
+                                        'success-rate': success_rate,
+                                        'failures': failures}))


You can compute the success_rate here. Or, you can pass 3 count instead of 1 rate + 2 count, which is more consistent.

Suggested change

def __write_stats(self, total_time, success_rate, failures, rendered_scenes):

success_rate = "{0}%".format(success_rate)

failures = "{0} out of {1} test scene(s)".format(failures, rendered_scenes)

self.file.write(self.__render(self.stats_template,

{'total-time': format_duration(total_time),

'success-rate': success_rate,

'failures': failures}))

def __write_stats(self, total_time, success_test_count, failed_test_count, total_test_count):

success_rate = int(success_test_count / total_test_count * 100.0)

total_time_text = format_duration(total_time)

success_rate_text = "{0}%".format(success_rate)

failures_text = "{0} out of {1} test scene(s)".format(failed_test_count, total_test_count)}))

self.file.write(self.__render(self.stats_template,

{'total-time': total_time_text,

'success-rate': success_rate_text ,

'failures': failures_text }))

What do you think ?

I think I mislead you in my last comment when suggesting to remove the suffix _str. My bad

I like your suggestion below of passing the values around in a class but I think it would still be useful to have the success rate included, since the number is used more than once and the calculation is quite long (you also need to check for 0 rendered scenes to avoid a divide by 0).

oktomus · 2020-12-01T12:07:11Z

scripts/runtestsuite/runtestsuite.py

@@ -452,9 +509,11 @@ def render_test_scene(args, logger, report_writer, project_directory, project_fi
 # --------------------------------------------------------------------------------------------------

 def render_test_scenes(script_directory, args):
-    rendered_scene_count = 0
+    rendered_scenes = 0


It should remain as it was

rendered_scenes suggest the variable contains multiple scenes, such as a list.

Suggested change

rendered_scenes = 0

rendered_scene_count = 0

oktomus · 2020-12-01T12:08:58Z

scripts/runtestsuite/runtestsuite.py

-    end_time = datetime.datetime.now()
-
-    success = 100.0 * passing_scene_count / rendered_scene_count if rendered_scene_count > 0 else 0.0
+    failures, rendered_scenes, success_rate, total_time = render_test_scenes(script_directory, args)


Some times, when you end up passing multiple argument everytime together, it's much easier to create an object.

Like class TestSuiteRunnerResult

Yup, I like it.

I'm thinking maybe something shorter (I notice Logger and not TestSuiteLogger for instance). How about TestResult or just Results?

Logger is something very common and speek for itself.

Results here is really only for the tests and doesn't speek for itself. You can't really use it ouside

russkev · 2020-12-02T00:44:05Z

Main changes are:

Results Class
- Contains methods for setting and accessing data needed for results statistics information
git_information method
- Refactored out from __write_header
Various name and style changes suggested by @oktomus

oktomus · 2020-12-02T11:47:45Z

Thanks for the changes @russkev :)

oktomus

Many thanks for that PR @russkev. This is a very good first contribution. Once merged, we should see your feature right here : http://testsuite.appleseedhq.net:8080/

The PR is ready to be merged for me, there is few styling changes I would suggest ( Results class name, method names like increment), but it's minor.

@dictoon Could you have a final look so that we can merge ?

oktomus · 2020-12-05T09:52:45Z

scripts/runtestsuite/runtestsuite.py

+    def __write_stats(self, results):
+        total_time_text = results.total_time()
+        success_rate_text = "{0}%".format(results.success_rate())
+        failures_text = "{0} out of {1} test scene(s)".format(
+            results.failure_count(), 
+            results.total_count())
+
+        self.file.write(self.__render(self.stats_template,
+                                       {'total-time': total_time_text,
+                                        'success-rate': success_rate_text,
+                                        'failures': failures_text}))


oktomus · 2020-12-05T09:53:52Z

scripts/runtestsuite/runtestsuite.py

+        success_rate_text = "{0}%".format(results.success_rate())
+        failures_text = "{0} out of {1} test scene(s)".format(
+            results.failure_count(), 
+            results.total_count())


Suggested change

results.total_count())

results.total_test_count())

total_count doesn't really mean anything on its own right ?

russkev · 2020-12-07T21:38:02Z

Thanks for the kind words @oktomus I'm happy to help. Also thank you for your detailed reviews. Having read Clean Code, I very much appreciate the importance of good naming so I'm thankful to have guidance in that area.

I just made a few tweaks. Mainly changing a few names and adding some comments.

Also, what do you think about also implementing a __report_success method in ReportWriter so that the report lists all tests that were run?

oktomus · 2020-12-08T21:59:43Z

Also, what do you think about also implementing a __report_success method in ReportWriter so that the report lists all tests that were run?

You mean including successful tests in the html report ? Right now, showing only the failed tests is very useful because you can easily see if something is wrong or not. For a complete report, there is always the txt report : http://testsuite.appleseedhq.net:8080/current-job/sandbox/tests/test%20scenes/testsuite_script_log.txt. But maybe you have a design in mind that allow to get all the tests, while still being able to quickly see the failures ? Although I'm not sure how seeing all tests could be used for

russkev · 2020-12-16T04:33:48Z

I have to admit, I haven't spent much time playing with the tool in practice so I'm not familiar with how it's used. I thought that it would be nice to have all the information from the tests in one nicely presented html file instead of split up over multiple documents.

The way I was thinking of doing it was to have the successful tests be just a single line and the failures would have the comparison images, maybe some red text, so that it would be easy to differentiate.

oktomus

We went through your code one last time with @dictoon and we have some minor suggestions. After that, we can merge it :) Thanks again, great work!

oktomus · 2021-02-02T12:17:22Z

scripts/runtestsuite/runtestsuite.py

+def command_is_valid(command):
+    try:
+        command_return = subprocess.check_call( 
+            # Ensure space seperated arguments are separate elements of an array 


Suggested change

# Ensure space seperated arguments are separate elements of an array

# Ensure space separated arguments are separate elements of an array

scripts/runtestsuite/runtestsuite.py

oktomus · 2021-02-02T12:21:10Z

scripts/runtestsuite/runtestsuite.py

@@ -262,8 +369,6 @@ def __make_update_command(self, output_filepath, reference_filepath):
            return 'copy /Y "{0}" "{1}"'.format(output_filepath, reference_filepath)
        else:
            return 'cp "{0}" "{1}"'.format(output_filepath, reference_filepath)
-
-
 # --------------------------------------------------------------------------------------------------


Reverting that change

Suggested change

# --------------------------------------------------------------------------------------------------

# --------------------------------------------------------------------------------------------------

oktomus · 2021-02-02T12:22:37Z

scripts/runtestsuite/runtestsuite.py

+        self.start_time = datetime.datetime.min
+        self.end_time = datetime.datetime.min
+
+    def increment_total_count(self):


Suggested change

def increment_total_count(self):

def increment_rendered_count(self):

oktomus · 2021-02-02T12:22:46Z

scripts/runtestsuite/runtestsuite.py

+    def increment_total_count(self):
+        self.rendered += 1
+
+    def increment_successes_count(self):


Suggested change

def increment_successes_count(self):

def increment_success_count(self):

oktomus · 2021-02-02T12:24:11Z

scripts/runtestsuite/stats_template.html

+            <h2>Summary</h2>
+            <table class="details">
+                <tr>
+                    <td>Success rate</td>


Suggested change

<td>Success rate</td>

<td>Success Rate</td>

oktomus · 2021-02-02T12:24:30Z

scripts/runtestsuite/header_template.html

+                    <td>Source commit</td>
+                    <td>{git-title}</td>
+                </tr>
+                <tr>
+                    <td>Source commit hash</td>
+                    <td>{git-hash}</td>


Suggested change

<td>Source commit</td>

<td>{git-title}</td>

</tr>

<tr>

<td>Source commit hash</td>

<td>{git-hash}</td>

<td>Source Commit</td>

<td>{git-title}</td>

</tr>

<tr>

<td>Source Commit Hash</td>

<td>{git-hash}</td>

russkev · 2021-02-18T22:42:54Z

Okay, I've made those changes and merged the latest master. Let me know if there's anything else I can do.

oktomus · 2021-02-19T15:20:40Z

Thanks a lot @russkev !

@dictoon I think we can merge it now. What do you say ?

dictoon · 2021-02-22T17:09:34Z

I think we can merge it now. What do you say ?

Yes!

Thanks @russkev for your work, and @oktomus for the reviews.

russkev added 5 commits November 24, 2020 17:46

initial commit

0ca8752

Git hash and title included in html report

7d1f445

Terminal stats output replicated in html report

b53a5af

cleanup

2a65b2b

Removed unecessary filed

eeba51f

oktomus reviewed Nov 29, 2020

View reviewed changes

russkev and others added 4 commits December 1, 2020 12:09

Update scripts/runtestsuite/runtestsuite.py

66cdf6e

Co-authored-by: Kevin Masson <hi@oktomus.com>

Update scripts/runtestsuite/header_template.html

2aec82f

Co-authored-by: Kevin Masson <hi@oktomus.com>

Renamed variables for clarity

cfc8618

More robust git information checking, etc.

0d21ad1

- Check scripts directory for git information instead of calling directory - Check that git is installed - Check that scripts directory is under git control - Variables renamed for clarity - Refactored various methods

Removed unecessary import

69e8b44

oktomus reviewed Dec 1, 2020

View reviewed changes

Implemented Results class and git_information method

4fa0f92

oktomus approved these changes Dec 5, 2020

View reviewed changes

russkev added 2 commits December 8, 2020 08:26

Tweaked naming and comments

6a4c8a0

Merge branch 'master' into kev-html-report

aaec0ac

oktomus reviewed Feb 2, 2021

View reviewed changes

russkev added 2 commits February 19, 2021 09:38

Minor name changes

96959ac

Merge branch 'master' into kev-html-report

7b9558d

oktomus mentioned this pull request Feb 22, 2021

Include commit hash (and possibly title) in test suite HTML report #2853

Closed

dictoon changed the title ~~HTML Report additions~~ Improve automatic test suite's HTML reports Feb 22, 2021

dictoon merged commit cf98638 into appleseedhq:master Feb 22, 2021

		git_hash = subprocess.check_output(['git', 'rev-parse', 'HEAD'])
		git_title = subprocess.check_output(['git', 'rev-parse', '--abbrev-ref', 'HEAD'])

		self.file.flush()

		def __write_stats(self, total_time, success, failures):

	# Ensure space seperated arguments are separate elements of an array
	# Ensure space separated arguments are separate elements of an array

	# --------------------------------------------------------------------------------------------------


	# --------------------------------------------------------------------------------------------------

	def increment_total_count(self):
	def increment_rendered_count(self):

	def increment_successes_count(self):
	def increment_success_count(self):

Improve automatic test suite's HTML reports #2903

Improve automatic test suite's HTML reports #2903

Conversation

russkev commented Nov 24, 2020

oktomus 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

russkev commented Dec 1, 2020

oktomus 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

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

russkev commented Dec 2, 2020

oktomus commented Dec 2, 2020

oktomus 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

russkev commented Dec 7, 2020

oktomus commented Dec 8, 2020 • edited Loading

russkev commented Dec 16, 2020

oktomus 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

russkev commented Feb 18, 2021

oktomus commented Feb 19, 2021

dictoon commented Feb 22, 2021

oktomus commented Dec 8, 2020 •

edited

Loading