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

General Intern API improvements #1041

Closed
jason0x43 opened this issue Jan 16, 2020 · 14 comments
Closed

General Intern API improvements #1041

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

Comments

@jason0x43
Copy link
Member

The goal of this issue is to consider how Intern's general testing API can be improved for version 5.

For example, Intern 4 presents its plugin API, which requires an intern global, as the default way to add user functionality and to access built in functionality such as test interfaces. For example, the canonical way to access the "object" interface is with

const { describe, it } = intern.getPlugin('interface.bdd');

This was done primarily to enable interoperability with browsers, which don't have native loaders. However, the non-loader case is more the exception than the rule, and the getPlugin API is a non-standard way to load modules. Many/most users tend to just import the object interface directly from intern/lib/interfaces/object, and rely on something like webpack to handle in-browser module loading.

So, one potential minor API improvement would be to make importing the testing interfaces simpler:

import { describe, it } from 'intern';
// or
import { describe, it } from 'intern/interfaces';

A more general improvement would be to clearly indicate what parts of Intern are intended to be consumed by users and which are internal (e.g., intern/lib/xxx is internal).

@jason0x43 jason0x43 added the discovery Research, no development label Jan 16, 2020
@aciccarello
Copy link
Contributor

I like the import syntax over using the global. I'm not sure what the import should be but I do like that the current getPlugin indicates the interface so maybe include that in the path?

import { describe, it } from 'intern/interfaces/bdd';

@jason0x43
Copy link
Member Author

We should consider renaming (or aliasing for now) functionalSuites to webdriverSuites. The term "webdriver" is in common usage, and its meaning may be clearer than "functional" to users.

@jason0x43 jason0x43 added the priority-high Most important label Jan 24, 2020
@jason0x43
Copy link
Member Author

jason0x43 commented Jan 31, 2020

Some tasks that would fall under this umbrella:

Use standard Promises

Getting rid of Task and CancellablePromise will allow Intern to work properly with native async/await support. It will also simplify the internal code in several places.

This includes updating the webdriver API. We may actually want to provide two APIs for a while, one allowing Command chaining and one that only deals with native promises. Async/await can significantly simplifies webdriver calls, but some users may still prefer the chaining approach. At the very least, it would be nice to not require everyone to rewrite all their webdriver tests.

De-emphasize the intern global

The global was included for compatibility with browser environments, where a native loader isn't generally available. It was described as the canonical way to deal with Intern. However, most users aren't using Intern in a loaderless environment, and modern libraries aren't typically consumed through globals.

Instead, the standard way to consume Intern resources should be through imports. There may still be situations where a global is required, but we should try to handle those behind-the-scenes when possible.

Intern should keep its plugin system for cases where working through modules isn't possible, and also to allow user code to be added to Intern's execution flow.

Improve some of Intern's default behaviors

Enable filterErrorStack by default.

Clean up Intern's public API

Decide what should be exposed and export it. Modules under /lib (at any level) are not part of the public API.

Improve how Intern's configuration is created and updated

Intern's configuration can come from several sources:

  • a JSON file, which may extend other JSON files, and may include child configs (which can extend other child configs)
  • environment variables
  • command line arguments
  • programmatically via intern.configure()

The various sources of config data are combined into a single config before tests are run. This combination currently happens in multiple places (_resolveConfig, loadConfig, ...), which can lead to unintuitive configuration results (see #877).

The rules for combining various config properties aren't consistent. In some cases we implicitly combine things (like node.suites with suites), while in other cases we require a + on the property name to indicate merging. We should make these rules more consistent.

Some tasks:

  • The code for configuration loading (when it reads in data from various sources) and resolution (when it applies override and merging rules) should be more clearly separated
  • Plugins should be loaded before configuration resolution is finalized, giving them a chance to affect the configuration. This will require a two-stage resolution: once to generate the initial config, and again after plugins have been loaded.
  • Make config merging rules consistent.

Configuration cleanup / improvements

Renaming config options

  • functionalSuites -> webdriverSuites
  • functionalBaseUrl -> webdriverBaseUrl
  • functionalCoverage -> webdriverCoverage
  • functionalRetries -> webdriverRetries
  • functionalTimeouts -> webdriverTimeouts

Restructing config options

  • Combine tunnel + tunnelOptions to work like loader
{
  "tunnel": {
    "name": "selenium",
    "options": {
      "drivers": [{"name": "chromedriver", "version": "80.1234"}]
    }
  }
}
  • Combine benchmark and benchmarkConfig
{
  "benchmark": {
    "enabled": false,
    "options": {
      "id": "foo",
      "mode": "baseline"
    }
  }
}

@maxbeatty
Copy link
Contributor

What's the thinking behind keeping webdriverOption* instead of moving towards a webdriver object with options inside like benchmark and tunnel?

{
  "webdriver": {
    "suites": "...",
    "baseUrl": "..."
  }
}

@jason0x43
Copy link
Member Author

Webdriver options

That's a good point, @maxbeatty, the webdriver options would probably be clearer (and less repetitive) as a group. That would give us (with the current set):

{
  "webdriver": {
    "suites": [],
    "baseUrl": "/",
    "coverage": false,
    "retries": 3,
    "timeouts": {}
  }
}

That might also help to distinguish the webdriver tests from the node tests and browser tests, which could alleviate some of the confusion that always seems to surround those.

That might make "suites" confusing, though. You could end up with

{
  "suites": "tests/unit/common/*.js",
  "node": {
    "suites": "tests/unit/node/*.js",
  },
  "browser": {
    "suites": "tests/unit/browser/*.js"
  },
  "webdriver": {
    "suites": "tests/webdriver/*.js"
  }

The current config semantics are that "suites" are run in all environments. In this case, though, that wouldn't include the "webdriver" environment. That may not be that big of a deal, or we may want to leave "webdriverSuites" as a separate option.

Benchmark config

The benchmark config could be flattened; it's mostly options with an enabled flag to take the place of the current benchmark option (it lets you disable benchmarking without having to comment out whatever options you've setup).

{
  "benchmark": {
    "enabled": false,
    "id": "foo",
    "mode": "baseline"
  }
}

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

More tasks...

Internalize the assertion API

Intern can continue making use of chai, but it should not expose it directly. Users should be able to import the assertion systems from a logical point:

import assert from 'intern/assertions/assert';
import expect from 'intern/assertions/expect';

Intern could also provide the API directly from the Intern object, a la Jest, although that may or may not be worthwhile.

describe('thing', () => {
  it('should do something', () => {
    intern.assertIsTrue(...);
  })
});

@jason0x43
Copy link
Member Author

Another potential task:

Remove TDD test interface

The TDD interface is a clone of the BDD interface (well, the other way around), the only difference is the method names. Aliasing names is trivial with modern JS syntax, so there's no real need for two separate interfaces. Since BDD is the standard, we should keep that one and remove tdd. The TDD interface could then be emulated with

import { describe as suite, it as test } from 'intern/src/core/lib/interfaces/bdd';

@jason0x43 jason0x43 removed this from the Intern 5.0 milestone Feb 7, 2020
@jason0x43
Copy link
Member Author

Actually, after some offline discussion, it's probably worth keeping the tdd interface since it is more convenient than having to alias bdd all the time.

On a related note, it has been proposed that the object interface be removed (or at least deprecated). Removing that one would offer some maintenance benefits; it is both the most difficult to implement (it still has some typing corner cases) and possibly the least used interface.

@jason0x43 jason0x43 self-assigned this Feb 14, 2020
@indolering
Copy link

I like the import syntax over using the global. I'm not sure what the import should be but I do like that the current getPlugin indicates the interface so maybe include that in the path?

Why? It's just more stuff to remember and more clutter. Is there overlap between the interfaces?

Intern can continue making use of chai, but it should not expose it directly.

Supporting async functionality transparently would be nice, Chai doesn't do this by default.

On a related note, it has been proposed that the object interface be removed (or at least deprecated). Removing that one would offer some maintenance benefits; it is both the most difficult to implement (it still has some typing corner cases) and possibly the least used interface.

Or modernize it to use ES6 classes and just run functions which aren't prefixed with an underscore.

@indolering
Copy link

I also don't like JSON config files, as you can't just comment out sections. Consider using a JS module which exports a default config object. 11ty has a cool hierarchical config system that uses deep merging of properties.

@jason0x43
Copy link
Member Author

jason0x43 commented Feb 16, 2020

Supporting async functionality transparently would be nice, Chai doesn't do this by default.

We could also bring in chai-as-promised, which which help out with that.

Or modernize it to use ES6 classes and just run functions which aren't prefixed with an underscore.

The object interface isn't meant to work like a class, and wouldn't map directly. Having a class-based interface would be worth looking into more, though.

I also don't like JSON config files, as you can't just comment out sections. Consider using a JS module which exports a default config object.

Using a JS and/or TS config would simplify certain operations, but it complicates the loading process, and it makes it easier for users to shoot themselves in the foot by creating a config that isn't loadable in some environment. (Like, a config written in TS or using ES6 features isn't going to work in IE.) That's definitely worth considering, though.

You actually can comment out sections in Intern's config, although you do have to worry about leaving trailing commas. 😄

@jason0x43
Copy link
Member Author

Why? It's just more stuff to remember and more clutter. Is there overlap between the interfaces?

I tend to agree. When you're writing potentially tens of test suites, there's some value in not having to import the same things over and over again.

A reasonable approach might be to install the bdd interface globally by default, and provide an interface config option. The interface option could be set explicitly to null or false, which would prevent any interface from being installed globally, or it could be set to one of the interfaces (bdd, tdd, object), which would install that interface.

For a bit more flexibility, we could have a globals option that could manage the interface, assertions, and possibly some mocking functionality.

{
  "globals": {
    "interface": "bdd",
    "assertions": "expect"
  }
}

As far as typing goes, the simplest approach would just be to type everything in the global namespace that could potentially be put there. Alternatively, Intern could provide .d.ts files that would declare the various features on the global namespace, and users could add the ones they cared about to the project's tsconfig.

Note that providing this kind of functionality wouldn't negatively impact users who chose to import interfaces. Users can currently do this by creating a plugin to install an interface globally (as Intern does in its own self tests), but it would make life just a bit easier.

@indolering
Copy link

Why? It's just more stuff to remember and more clutter. Is there overlap between the interfaces?

I tend to agree. When you're writing potentially tens of test suites, there's some value in not having to import the same things over and over again.

Sorry, could have said this better: I prefer the import style but I don't like breaking the interfaces out into paths: import { describe, it } from 'intern' vs import { describe, it } from 'intern/interfaces/bdd'.

But I understand your point, especially if you have to break your tests into individual files. I just tried writing BabelJS transpilers for all of my ESM based tests and it's a huge PITA.

A reasonable approach might be to install the bdd interface globally by default, and provide an interface config option. The interface option could be set explicitly to null or false, which would prevent any interface from being installed globally, or it could be set to one of the interfaces (bdd, tdd, object), which would install that interface.

What about having magical globals for scripts and Node but requiring imports for the ESM version? But that makes test reuse across environments a PITA ... sigh. It's probably best to punt on this until ESM is the norm and just do whatever is easiest and the most reusable across scripts/ESM/and Node.

@jason0x43
Copy link
Member Author

Closing as discovered.

Outcomes:

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
None yet
Development

No branches or pull requests

4 participants