Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reporters review #1023

Closed
jason0x43 opened this issue Jan 13, 2020 · 7 comments
Closed

Reporters review #1023

jason0x43 opened this issue Jan 13, 2020 · 7 comments
Assignees
Labels
discovery Research, no development priority-high Most important

Comments

@jason0x43
Copy link
Member

The goal of this issue is to review Intern's existing reporters (what's there and how they work) and suggest which ones we could drop and what additional ones we might need, as well as how they might be better implemented.

For example, there isn't current a consistent way to configure reporters from the main config; you can provide a reporter description with options to reporters, but each reporter may take different options.

Some questions to answer:

  • What kinds of reporters do we need? (console, browser, CI, coverage, ...)
  • Which existing reporters could we discard and/or combine?
  • How should reporters be configured? Or more specifically, should reporters be configurable at all from the user perspective?
@jason0x43 jason0x43 added the discovery Research, no development label Jan 13, 2020
@devpaul
Copy link
Contributor

devpaul commented Jan 22, 2020

We did some work in #1014 with trying to normalize filename/directory information. We learned that while the Istanbul reporters have directory and filename properties, those values are simply part of two different bags handed off to the reporter where they may or may not be used. This is part of the Coverage reporter, which serves as a parent to reporters: Cobutura, HtmlCoverage, JsonCoverage, Lcov, TextCoverage (parent to Simple, Runner, Pretty). Some of these only output to the console making filename and directory meaningless.

@jason0x43 and I also discussed moving toward a factory-based creation of reporters and treating them as a bag of event handlers. Many of the current classes aren't extensible in any meaningful way and really only serve as a wrapper themselves. Extension could be explicitly defined with functions passed to the factory.

Regarding normalizing filename and directory usage. I think there needs to be a way to define a base directory for reporter output (which may default to .). This would be the easiest way to globally define output for reporters. Then a directory property for each reporter would output to a default location, which may be overridden with a relative or absolute path (relative to the base directory). Finally filename would be available for reporters that output a single filename. When present, each would have a default that would allow for reporters to be configurationless if desired.

@jason0x43
Copy link
Member Author

Some potential tasks we have so far:

  • Convert reporters to factory-created event handlers.
  • Standardize the option formats accepted by the built-in reporters, particularly around file handling.
  • Standardize the behavior of file-generating reporters.All of them should work with no configuration. The output directory should be configurable for all of them, and the output file name should be configurable for reporters that generate a single file.
  • Add a reportPath top-level config option to Intern and make sure all file-generating reporters respect it. This will serve as a default, and can be overridden in individual reporters.

@jason0x43
Copy link
Member Author

From a user-interface perspective, Intern could be made easier to use by move away from exposing the built in reporters as distinct entities. Instead, users should see Intern as generating various types of output. For example, by default it outputs to the console, and there should be various options to control this output. It can also output a Junit report, or an HTML coverage report, or various other things. To enable reporting functionality, the user should just need to include a config option for it. We might move to something like this:

// intern.json
{
  "output": {
    "hidePassed": true,
    "hideCoverage": true,
    "htmlCoverage": {},
    "junit": {
      "path": "test-reports"
    }
  }
}

In the snippet above, a single "output" property controls Intern's output. The default output can be managed through top-level properties such as "hidePassed". Other report types can be enabled by providing some config for them. "htmlCoverage": {} would generate an HTML coverage report with the default options. The "junit" entry would generate JUnit output with the given options.

From the command line, report control could work similarly. A command line providing the same config as above might look like:

> intern --output-hide-passed --output-hide-coverage --output-html-coverage --output-junit-path test-reports

Maybe the output isn't necessary in all cases.

@jason0x43 jason0x43 added this to the Intern 5.0 milestone Feb 3, 2020
@jason0x43
Copy link
Member Author

Review

Intern currently has the following reporters:

  • Benchmark - evaluate and display benchmark results in a Node console
  • Cobertura - generate a cobertura coverage report file
  • Console - display test results to browser console
  • Dom - display test results in the DOM during webdriver tests
  • Html - display test pretty, somewhat interactive results in the browser
  • HtmlCoverage - generate an HTML coverage report (multiple files)
  • JsonCoverage - generate a JSON coverage report file
  • JUnit - generate a JUnit test result file
  • Lcov - generate an lcov coverage report file
  • Pretty - display test results in the Node console in a "pretty" format
  • Runner - display test results in the Node console
  • Simple - display test results in the node console in a simpler format
  • TeamCity - generate a TeamCity coverage report file
  • TextCoverage - display a textual coverage report in the Node console

Users must explicitly select reporters using the reporters config option. Multiple reporters may be enabled at one time.

Reporters are currently class-based, all inheriting from Reporter, and many from Coverage. At their core, though, reporters are just event listeners.

Tasks

Combine Simple, Runner, and Pretty

There's no need for three separate Node console reporters. Create a single reporter for Node output with options to change its behavior. At a high level, use the Runner reporter's basic format, but with multi-session support similar to the Pretty reporters. Make reasonable use of colors and font family (bold and italic), but allow these to be easily disabled.

Convert reporters to simple event handlers

We're not getting any real benefit from the class hierarchy. Scrap it and reimplement the reporters as event listeners with some shared utility functions.

Reporters would be created with factory functions, potentially something like this:

// lib/reporters/node
export function createReporter(executor: Executor, options: NodeOutputOptions): Handle {
  const listeners = [];

  listeners.append(executor.on('testStart', test => {
    // ...
  }));

  listeners.append(executor.on('suiteEnd', suite => {
    // ...
  }));

  return {
    destroy() {
      // remove listeners
    }
  };
}

Simplify reporter configuration

This one's more about the concept than significantly changing the code... Replace the reporters config option with output. Code-wise this will look somewhat similar to the existing option, but conceptually it puts the focus on Intern's output rather than on loading distinct reporter modules.

By default, Intern will output to the console in whatever environment it's running in (and possibly to the DOM for remote webdriver sessions). If the user wants to set an option on Intern's output, they can set it on the output property. If they want to enable some additional form of output, that can also happen on output.

Custom reporters will simply be plugins, and will use the standard plugin configuration system.

Combine the coverage reporters

Since we're hiding the internal reporter structure from users, we could simplify the coverage reporters. Most of them are just deferring everything to the coverage base class, and through that to Istanbul. Just combine them all into a single module and let the Node executor create what's needed. For example, if Intern was configured with

{
  "output": {
    "htmlcoverage": {},
    "lcov": { "filename": "coverage-report.lcov" }
  }
}

Intern could just create multiple instances of a coverage reporter (notional syntax):

// executors/Node
import { createReporter as createCoverageReporter } from 'lib/reporters/coverage';
// ...
for (const name in supportedReporters) {
  if (name in this.config.output) {
    createCoverageReporter(this, name, this.config.output[name]);
  }
}

@jason0x43 jason0x43 added this to In progress in Intern 5 Feb 7, 2020
@jason0x43 jason0x43 removed this from the Intern 5.0 milestone Feb 7, 2020
@devpaul
Copy link
Contributor

devpaul commented Feb 7, 2020

This is a great approach to modernizing reporters.

A couple things:

  • I assume that custom reporters will still be registered ASAP so error output can be captured?
  • Can createReporter be an async factory function? This will allow the reporter factory to do things like start up a server and establish a connection or connect to a remote server before Intern starts. Something like this would be useful for reporting results to a GUI or IDE.
  • Is there a way for a reporter to finalize its output asynchronously? i.e. flush file outputs and send farewell messages to servers?

@jason0x43
Copy link
Member Author

I assume that custom reporters will still be registered ASAP so error output can be captured?

Custom reporters will still just be plugins, and plugins are loaded very early in the testing process. I was actually going to try to move them back even further so they'd have a chance to affect the resolved configuration (or break up configuration resolution around plugin loading).

Can createReporter be an async factory function?

The createReporter pattern is for internal reporters; external reporters will be plugins, which can already perform async actions during the initial plugin registration process or in an event listener like runStart.

import { registerPlugin } from 'intern';
import { doAsyncStuff } from 'async-stuff';
registerPlugin('my-reporter', async () => {
  await doAsyncStuff();
  intern.on('testStart', test => {
    console.log(`started ${test.id}`);
  });
});

We could definitely make createReporter async, although none of the internal reporters currently do any async startup.

Is there a way for a reporter to finalize its output asynchronously?

Every event listener is async, so a reporter can just return a Promise from a late event like runEnd.

However... Intern isn't currently waiting for all the event handlers to finish, so while the process would remain open during cleanup actions, Intern wouldn't be paying attention any longer. One of the the planned general API updates is to integrate the event handlers into the execution process.

@jason0x43
Copy link
Member Author

Closing this as discovered.

Outcomes:

Intern 5 automation moved this from In progress to Done Apr 14, 2020
@github-actions github-actions bot added this to Done in Development Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discovery Research, no development priority-high Most important
Projects
Intern 5
  
Done
Development

No branches or pull requests

2 participants