GNOME Bugzilla – Bug 775776
Add GjsCoverage to gjs-1.0 public API
Last modified: 2016-12-21 01:58:37 UTC
If embedders (such as gnome-shell) want to enable code coverage in their tests, then we should allow them to create a GjsCoverage object.
Created attachment 341703 [details] [review] coverage: Use GFile internally instead of paths Some paths may be resource paths, so it's better to use GFile rather than using g_file_new_for_commandline_arg() to guess. (Although, we still must use g_file_new_for_commandline_arg() in some of the tests.)
Created attachment 341704 [details] [review] coverage: Coverage without cache internal-only This removes the gjs_coverage_new_for_cache() constructor. Instead, gjs_coverage_new() always creates a cache in the default location, since it is unlikely that you would ever want to run coverage without a cache except in GjsCoverage's unit tests. For the tests, this adds two new constructors to coverage-internal.h, _gjs_coverage_new_internal_with_cache() and _gjs_coverage_new_internal_without_cache(). As for creating a GjsCoverage object directly with g_object_new(), you can unfortunately still get whatever behaviour you want from the public API. If you don't specify a "cache" property at construct time, you get the default cache. If you do specify it and it is NULL, you get no cache.
Created attachment 341705 [details] [review] coverage: Use correct constness for prefixes Both levels of pointers should be const, or else you have to add a cast in C++. (You have to add a cast in C in any case.)
Created attachment 341706 [details] [review] console: Don't leak coverage object It matters very little because the program is about to exit anyway, but this should keep leak checkers quiet.
Created attachment 341707 [details] [review] coverage: Expose GjsCoverage in public API This exposes the GjsCoverage type, its constructor gjs_coverage_new(), and its method gjs_coverage_write_statistics(), for GJS embedders who would like to gather code coverage statistics. (gnome-shell doesn't currently do this, but it could.)
Review of attachment 341703 [details] [review]: ::: gjs/coverage.cpp @@ +937,3 @@ NULL, &error)) { + char *uri = g_file_get_uri(file); Nitpick: use path for warning/critical messages, since it's also used elsewhere? @@ +1393,2 @@ out: + g_clear_object(&file); You can call g_object_unref() unconditionally, because file will always be != NULL here now. @@ +1460,3 @@ out: g_clear_error(&error); + g_clear_object(&file); I think file will always be != NULL here now.
Review of attachment 341704 [details] [review]: Looks good.
Review of attachment 341705 [details] [review]: Looks good.
Review of attachment 341706 [details] [review]: Sure
Review of attachment 341707 [details] [review]: Looks good.
Created attachment 341908 [details] [review] coverage: Use GFile internally instead of paths Some paths may be resource paths, so it's better to use GFile rather than using g_file_new_for_commandline_arg() to guess. (Although, we still must use g_file_new_for_commandline_arg() in some of the tests.)
Comment on attachment 341706 [details] [review] console: Don't leak coverage object Attachment 341706 [details] pushed as 1545a1e - console: Don't leak coverage object
(In reply to Cosimo Cecchi from comment #6) > Review of attachment 341703 [details] [review] [review]: > > ::: gjs/coverage.cpp > @@ +937,3 @@ > NULL, > &error)) { > + char *uri = g_file_get_uri(file); > > Nitpick: use path for warning/critical messages, since it's also used > elsewhere? I don't agree; well, I do agree that it would be more readable, but the reason for this patch in the first place was because the GFiles may be things that only have URIs and not paths. For example, for a resource:// URI you'd get NULL for g_file_get_path().
(In reply to Philip Chimento from comment #13) > I don't agree; well, I do agree that it would be more readable, but the > reason for this patch in the first place was because the GFiles may be > things that only have URIs and not paths. For example, for a resource:// URI > you'd get NULL for g_file_get_path(). My point was not about readability, but consistency. I agree that URIs are better because you can have resources; in that case, please change all the other similar messages to consistently use URIs.
Created attachment 341991 [details] [review] coverage: Use GFile internally instead of paths Some paths may be resource paths, so it's better to use GFile rather than using g_file_new_for_commandline_arg() to guess. (Although, we still must use g_file_new_for_commandline_arg() in some of the tests.)
Created attachment 341992 [details] [review] coverage: lcov output directory is construct prop Change the lcov output directory to be a construct property of GjsCoverage. Instead of putting the coverage cache in the current directory by default, it is instead put in the lcov output directory. This required rewriting many of the coverage tests to use GFile internally so this simplifies some of the test fixtures as well. We put everything in one temporary directory and delete that directory at teardown, rather than using randomized names for each temporary file. Consolidating some fixtures allows getting rid of much of the fixture->base_fixture.base_fixture.whatever pattern, which was hard to read.
Created attachment 341993 [details] [review] coverage: Coverage without cache internal-only This removes the gjs_coverage_new_for_cache() constructor. Instead, gjs_coverage_new() always creates a cache in the default location, since it is unlikely that you would ever want to run coverage without a cache except in GjsCoverage's unit tests. For the tests, this adds two new constructors to coverage-internal.h, _gjs_coverage_new_internal_with_cache() and _gjs_coverage_new_internal_without_cache(). As for creating a GjsCoverage object directly with g_object_new(), you can unfortunately still get whatever behaviour you want from the public API. If you don't specify a "cache" property at construct time, you get the default cache. If you do specify it and it is NULL, you get no cache.
Created attachment 341994 [details] [review] coverage: Use correct constness for prefixes Both levels of pointers should be const, or else you have to add a cast in C++. (You have to add a cast in C in any case.)
Created attachment 341995 [details] [review] coverage: Expose GjsCoverage in public API This exposes the GjsCoverage type, its constructor gjs_coverage_new(), and its method gjs_coverage_write_statistics(), for GJS embedders who would like to gather code coverage statistics. (gnome-shell doesn't currently do this, but it could.)
Review of attachment 341991 [details] [review]: Feel free to push with these fixed. ::: gjs/coverage.cpp @@ +1420,2 @@ if (!checksum) { + char *filename = g_file_get_path(file); Use get_file_identifier() here? @@ +1457,3 @@ NULL, &error)) { + char *filename = g_file_get_path(file); Use get_file_identifier() here?
(In reply to Cosimo Cecchi from comment #20) > Review of attachment 341991 [details] [review] [review]: > > Feel free to push with these fixed. > > ::: gjs/coverage.cpp > @@ +1420,2 @@ > if (!checksum) { > + char *filename = g_file_get_path(file); > > Use get_file_identifier() here? > > @@ +1457,3 @@ > NULL, > &error)) { > + char *filename = g_file_get_path(file); > > Use get_file_identifier() here? In both these cases the GFile is created from a path passed from a JS string, so it's not necessary because g_file_get_path() won't return NULL. Do you think I should just do it anyway for consistency?
(In reply to Philip Chimento from comment #21) > In both these cases the GFile is created from a path passed from a JS > string, so it's not necessary because g_file_get_path() won't return NULL. > Do you think I should just do it anyway for consistency? I think so, but I will leave it up to you if you feel strongly about it one way or the other.
Review of attachment 341992 [details] [review]: ::: gjs/coverage.cpp @@ +364,3 @@ + char *relpath = g_file_get_relative_path(ancestor, child); + if (relpath) + return relpath; I think ancestor is leaked in this code path @@ +374,3 @@ + g_object_unref(root); + if (child_path) + return child_path; Do you need both this and the code path below? In which case would we take one or the other? Maybe you can add a comment. @@ +780,3 @@ + * g_file_new_for_commandline_arg() to disambiguate between URIs and + * filesystem paths. */ + GFile *source = g_file_new_for_commandline_arg(file_statistics->filename); Looks like this is leaked @@ +1714,3 @@ break; + case PROP_OUTPUT_DIRECTORY: + priv->output_dir = G_FILE(g_value_dup_object(value)); No need for the static cast here? @@ +1817,3 @@ * @coverage_prefixes: (transfer none): A null-terminated strv of prefixes of files to perform coverage on * coverage_data for + * @output_path: A path to write coverage information to. Scripts which were This is not output_path anymore. ::: test/gjs-test-coverage.cpp @@ +64,3 @@ +{ + GFileEnumerator *files = + g_file_enumerate_children(dir, "standard::*", G_FILE_QUERY_INFO_NONE, I think you can just use "standard::type" here. @@ +72,3 @@ + !file || !info) + break; + while (TRUE) { Use g_file_info_get_file_type() @@ +1595,3 @@ { + char *coverage_script_filename = g_file_get_path(coverage_script); + char *retval = g_strdup_printf("const JSUnit = imports.jsUnit;\n" Indentation of following lines
Review of attachment 341993 [details] [review]: Looks good
Review of attachment 341994 [details] [review]: Sure
Review of attachment 341995 [details] [review]: Looks good
Review of attachment 341991 [details] [review]: ::: test/gjs-test-coverage.cpp @@ +1654,3 @@ + + char *hash_string_no_quotes = gjs_get_file_checksum(resource); + g_object_unref(resource); My only suggestion here would be to create wrapper functions to get the checksum/mtime from a path without having to create an intermediate GFile* during the tests, though it appears that this only happens once and may not be worth it.
(In reply to Sam Spilsbury from comment #27) > Review of attachment 341991 [details] [review] [review]: > > ::: test/gjs-test-coverage.cpp > @@ +1654,3 @@ > + > + char *hash_string_no_quotes = gjs_get_file_checksum(resource); > + g_object_unref(resource); > > My only suggestion here would be to create wrapper functions to get the > checksum/mtime from a path without having to create an intermediate GFile* > during the tests, though it appears that this only happens once and may not > be worth it. In the following commit I stop storing those files as pathnames and instead store them as GFiles, so that's probably not necessary.
(In reply to Cosimo Cecchi from comment #23) > Review of attachment 341992 [details] [review] [review]: > @@ +1714,3 @@ > break; > + case PROP_OUTPUT_DIRECTORY: > + priv->output_dir = G_FILE(g_value_dup_object(value)); > > No need for the static cast here? Indeed. The output directory is not supposed to be NULL, so G_FILE() works. The previous property needs the static cast because it's allowed to be NULL, on which G_FILE() would log a critical.
Comment on attachment 341991 [details] [review] coverage: Use GFile internally instead of paths Attachment 341991 [details] pushed as 0110886 - coverage: Use GFile internally instead of paths
Created attachment 342235 [details] [review] coverage: lcov output directory is construct prop Change the lcov output directory to be a construct property of GjsCoverage. Instead of putting the coverage cache in the current directory by default, it is instead put in the lcov output directory. This required rewriting many of the coverage tests to use GFile internally so this simplifies some of the test fixtures as well. We put everything in one temporary directory and delete that directory at teardown, rather than using randomized names for each temporary file. Consolidating some fixtures allows getting rid of much of the fixture->base_fixture.base_fixture.whatever pattern, which was hard to read.
Review of attachment 342235 [details] [review]: Feel free to push with these two nits. ::: gjs/coverage.cpp @@ +1175,3 @@ + if (!g_file_make_directory_with_parents(priv->output_dir, NULL, &error)) { + if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_EXISTS)) { + g_critical("Could not create coverage output: %s", error->message); The error is leaked in this code path ::: test/gjs-test-coverage.cpp @@ +1873,1 @@ + GFile *mock_resource = g_file_new_for_uri("resource:///org/gnome/gjs/mock/cache/resource.js"); Is this ever unreffed?
Attachment 341993 [details] pushed as 79d0b86 - coverage: Coverage without cache internal-only Attachment 341994 [details] pushed as be554f0 - coverage: Use correct constness for prefixes Attachment 341995 [details] pushed as 6f5ccf7 - coverage: Expose GjsCoverage in public API Attachment 342235 [details] pushed as ebf0cf0 - coverage: lcov output directory is construct prop