Closed Bug 916265 Opened 6 years ago Closed 5 years ago

Implement a jsm to generate log messages in the mozlog.structured logging format for tests

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Log4Moz supports structured output, but does not have a notion of the common message formats that will be produced by test harnesses in javascript. This bug is about implementing a test logging library for the convenience of reporting test results from harnesses written in javascript, in the format to be determined in bug 916260.

Some questions that came up during discussion of this:

Should this be a part of gecko, or a standalone module?

Will formatting results for human consumption be a requirement of this module? This was danced about a bit in bug 896087 - it was decided to save the repeated logic and leave human formatting to python, but this could be revisited.
(In reply to Chris Manchester [:chmanchester] from comment #0)
> Should this be a part of gecko, or a standalone module?

I'd be fine with putting it in TESTING_JS_MODULES, since it's only needed for test harnesses.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Implement a jsm supporting common structured log operations for test harnesses → Implement a jsm to generate log messages in the mozlog.structured logging format for tests
Blocks: 1033126
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
This is a start based on akachkach's work on mochitests. I've working with this for xpcshell.
Comment on attachment 8449444 [details] [diff] [review]
Implement a jsm to output structured log messages.

Review of attachment 8449444 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/modules/TestLogger.jsm
@@ +22,5 @@
> +this.TestLogger = function (name, dumpFun=dump, mutators=[]) {
> +  this.name = name;
> +  this._dumpFun = dumpFun;
> +  this._mutatorFuns = mutators;
> +  this._testsStarted = new Set();

I wonder if there's a better name for this that conveys the idea of "tests that are started but not finished".

@@ +74,5 @@
> +  this._logData("test_end", data);
> +}
> +
> +TestLogger.prototype.suiteStart = function (tests, runinfo) {
> +  runinfo = typeof runinfo !== "undefined" ? runinfo : null;

Aren't you using the default arguments thing above (I assume that's valid ES6, I didn't check). Why not here?

@@ +142,5 @@
> +  for (let field in data) {
> +    allData[field] = data[field];
> +  }
> +
> +  for (let fun of this._mutatorFuns) {

Out of interest, what's the use case here?
Attachment #8449444 - Flags: feedback?(james) → feedback+
(In reply to James Graham [:jgraham] from comment #4)
> Comment on attachment 8449444 [details] [diff] [review]
> Implement a jsm to output structured log messages.
> 
> Review of attachment 8449444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/modules/TestLogger.jsm
> @@ +22,5 @@
> > +this.TestLogger = function (name, dumpFun=dump, mutators=[]) {
> > +  this.name = name;
> > +  this._dumpFun = dumpFun;
> > +  this._mutatorFuns = mutators;
> > +  this._testsStarted = new Set();
> 
> I wonder if there's a better name for this that conveys the idea of "tests
> that are started but not finished".

Yes, that makes sense. I was also thinking the module and logger class should have "structured" somewhere in the name.

> 
> @@ +74,5 @@
> > +  this._logData("test_end", data);
> > +}
> > +
> > +TestLogger.prototype.suiteStart = function (tests, runinfo) {
> > +  runinfo = typeof runinfo !== "undefined" ? runinfo : null;
> 
> Aren't you using the default arguments thing above (I assume that's valid
> ES6, I didn't check). Why not here?

An oversight.

> 
> @@ +142,5 @@
> > +  for (let field in data) {
> > +    allData[field] = data[field];
> > +  }
> > +
> > +  for (let fun of this._mutatorFuns) {
> 
> Out of interest, what's the use case here?

I found this convenient for xpcshell, because we start up head.js per test file, and the logger class always knows which test file / thread id / etc. to append to a message, reducing a little boilerplate for each message. This might not be general enough to warrant inlcusion here.

I also wanted to highlight that this supports stuffing extra (structured) data into unstructured log actions. I think this is pretty convenient, but might be a dubious design choice.
Some small fixups and added basic tests.
Attachment #8449444 - Attachment is obsolete: true
Attachment #8449697 - Flags: review?(ted)
Unbitrot (use moz.build for TESTING_JS_MODULES)
Attachment #8452573 - Flags: review?(ted)
Attachment #8449697 - Attachment is obsolete: true
Attachment #8449697 - Flags: review?(ted)
Better tests, updated to include a parameter for exception stacks for test_status and test_end messages.
Attachment #8456443 - Flags: review?(ted)
Attachment #8452573 - Attachment is obsolete: true
Attachment #8452573 - Flags: review?(ted)
Comment on attachment 8456443 [details] [diff] [review]
Implement a jsm to output structured log messages.

Review of attachment 8456443 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/modules/StructuredLog.jsm
@@ +16,5 @@
> + *        The name of the logger to instantiate.
> + * @param dumpFun
> + *        An underlying function to be used to log raw messages.
> + * @param mutators
> + *        An array of functions used to add global context to log messages.

I feel like you could clarify these parameters a little bit, like what the functions should expect as an argument etc.

::: testing/modules/tests/xpcshell/test_structuredlog.js
@@ +25,5 @@
> +  let addFileName = function (data) {
> +    data.source_file = "test_structuredlog.js";
> +  }
> +
> +  Components.utils.import("resource://testing-common/StructuredLog.jsm", this);

You don't need the "this" here, that's implied if you leave it off. Also, I'd stick this import at the top of the function, it makes it more obvious.
Attachment #8456443 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/e8501234d9e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.