fix(testing): convert testPathPattern to an array

[dgsmith2]

Per jestjs/jest#5066, the testPathPattern parameter to the CLI is an array

[dgsmith2]

I'm stumped by the failing test because locally it passes.

[catfireparty]

@dgsmith2 - It's failing because your commit message on 9d97812 ("Used the correct typing for string array; Updated tests for type change") doesn't match our commit message convention.

Unless the two commits are distinct, I suggest squashing them both into the later one.

[dgsmith2]

@jdpearce, I squashed the commits and force-pushed. The build errors are now due to network issues between Travis CI and Heroku. Can these be manually triggered or do I need to make another change for an automatic trigger?

[catfireparty]

@dgsmith2 I've restarted that build for you. Hopefully it runs properly this time... 🤞

[dgsmith2]

@jdpearce success! 🎉

[catfireparty]

@dgsmith2 - Now that I've had a look through this, it occurs to me, isn't this going to be a breaking change?

What you've done looks good, but would it be a good idea to write a migration for this change?

Either that, or can we make it take either an array or a string?

@vsavkin, what do you think?

[vsavkin]

@dgsmith2 will it break existing workspaces when they update?

[dgsmith2]

@vsavkin the motivation behind this PR is to address the "display problem" in #1693 (comment)

Presently, the builder parses testPathPattern as a string and passes it to Jest. Jest eventually runs a spread (...) on the value when normalizing arguments. Thus an intended pattern of some/test becomes a pattern of s|o|m|e|/|t|e|s|t.

This fix eliminates the processing of additional (potentially all) tests only to have them skipped.

That said, it may be possible there is an edge case in which a spread of the value interpretted as a string is providing an Nx user the behavior they expect. However, I presently believe it to be improbable.

[catfireparty]

@dgsmith2 - while it's unlikely that anyone is relying on this behaviour, what will happen if someone has testPathPattern as a string and the builder tries to read it as an array?

Should we fix those workspaces such that testPathPattern: 'somepattern' becomes testPathPattern: ['somepattern']? I think this would be a relatively straightforward migration to write...question is, is it worth it?

[dgsmith2]

Perhaps I'm missing something, but there is no need to migrate. Changing it to an array means it will allow multiple values occurences of the argument (e.g., ng test --testPathPattern=foo --testPathPattern=bar)

The command my IDE is running is no different than before my fix (single instance of the testPathPattern argument). The only difference is the way the value(s) is stored in the builder and thus passed to Jest.

[catfireparty]

@dgsmith2 - you've changed the schema.json, this also defines the valid values in a workspace.json/angular.json. Since string is no longer a valid value after this change, we should arguably migrate (fix) those workspace files, no?

See, for example, /packages/jest/src/migrations/

[dgsmith2]

@jdpearce, I will be glad to take a look at adding a migration. As a new contributer, I'm happy to defer to the experience of others in determining what is necessary for this PR.

[dgsmith2]

Seems like I need to create a migration of angular.json to locate "builder": "@nrwl/jest:jest" and alter its options such that any occurence of the "testPathPattern": "a/path" key/value pair becomes "testPathPattern": [ "a/path" ].

Is there a naming convention for migrations?

[alfaproject]

Why not just support string and array? If it's a string, just convert to an array of 1 element

[dgsmith2]

@alfaproject, forgive me as I'm not completely familiar with it, but does the Angular CLI builder schema permit unioned types?

[vsavkin]

@dgsmith2 Thanks again for your contribution!

To answer your question: the schema permits union types.

But I'd only use the union type of jest itself supports both string and arrays. If Jest doesn't, I'd write a migration update angular.json/workspace.json.

This is an example of a migration that does something similar:

import { updateWorkspace } from '../../utils/workspace';
import { join, JsonArray, normalize } from '@angular-devkit/core';

const addExcludes = updateWorkspace(workspace => {
  workspace.projects.forEach(project => {
    project.targets.forEach(target => {
      if (target.builder !== '@angular-devkit/build-angular:tslint') {
        return;
      }
      const exceptRootGlob = '!' + join(normalize(project.root), '**');

      if (!target.options.exclude) {
        target.options.exclude = [];
      }

      if (!(target.options.exclude as JsonArray).includes(exceptRootGlob)) {
        (target.options.exclude as JsonArray).push(exceptRootGlob);
      }
    });
  });
});

export default function() {
  return addExcludes;
}

You can throw a migration into the update-8-6-0 folder.

[dgsmith2]

@jdpearce and @vsavkin, I was finally able to circle back around to this PR and add the requested migration.

Out of curiousity, once this PR is merged, what release schedule will it be under? I'm lead on a team tasked with incorporating Nx into our existing app. Once we roll it out I'd like to be able to inform developers when they can expect the test performance gains that result from this fix.

Thanks for all the help and direction you two offerred!

[catfireparty]

Awesome work, thanks for this, @dgsmith2!

I think @vsavkin will have a better idea of release schedule, but we're releasing pretty often at the moment, so I imagine it would be out within a week or two.

[github-actions]

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.