Replace `Settings` class by `getSettings` function #14

stephtr · 2019-07-14T21:50:45Z

Until now Settings.settings had been populated by the constructor using Jest's default values.
This however had a few disadvantages:

In order to get the default values from Jest (one string and one string array), we had a dependency on jest-config, which by itself pulled in a large amount of further dependencies.
In my opinion there are hardly any reasons why one would need Jest's default values. Usually one is interested in the precise settings being used in a specific workspace. Returning some generic default values only makes someone potentially forget about calling getConfigs.

As an alternative I propose to set Settings.settings to undefined instead (and Settings.configs to an empty array), until one of the getConfigs functions has been called. This will be a breaking change and if accepted, should probably wait until the next major version.

yarn.lock

seanpoulter · 2019-07-14T22:44:48Z

If we're starting to refactor this, should we talk about refactoring Settings.settings and Settings.configs in the same breaking change?

It seems like smelly code to have a settings member in as class representing settings:

new Setttings().settings.testMatch`

The other confusing thing is if we've got a class representing settings, why do we have an array of other settings (configs)? Can we rename something so it's more clear what's going on?

stephtr · 2019-07-15T05:29:40Z

What about:

export interface ConfigRepresentation {
  testRegex: string | string[];
  testMatch: string[];
  jestVersionMajor: number | null;
};

export class Settings extends EventEmitter {
  constructor(workspace: ProjectWorkspace, options?: Options);
  getSettings(): Promise<ConfigRepresentation[]>;
}

In principle we could move this getSettings function also to the ProjectWorkspace object.

rossknudsen · 2019-07-15T09:32:39Z

Definitely keen to see Settings.getSettings return a Promise. The current sync callback is not ideal:

const settings = new Settings({/* ... */});

const getConfig = (): Promise<void> => {
    return new Promise(resolve => {
        settings.getConfig(() => {
            resolve();
        });
    });
};
// wait for Promise to return so we have the correct settings for testMatch and testRegex.
await getConfig();

seanpoulter · 2019-07-15T13:23:11Z

That's way less confusing for me @stephtr. 😌
I'm not sure about moving things to the ProjectWorkspace object.

connectdotz · 2019-07-15T22:54:01Z

chime in my 2 cents, I think there is a great opportunity to move this whole class into a single stateless function, let's say getConfig(workspace: ProjectWorkspace, options?: Options) => Promise<ConfigRepresentation>:

other than the configRepresentation, all states are internal and used in 1 function. The configRepresentation will be returned to caller thus no need to be kept here.
the current workflow is convoluted: create a class => call geteConfig => once the callback is invoked, come back and get the "settings"... This can all be simplified into 1 call above.

stephtr · 2019-07-18T06:51:22Z

I now ended up with (the tests are still to be updated):

type Glob = string;

type ProjectConfiguration = {
  testRegex: string | Array<string>,
  testMatch: Array<Glob>,
};

type JestSettings = {
  jestVersionMajor: number,
  configs: ProjectConfiguration[],
};

function parseSettings(text: string, debug: Boolean = false): JestSettings {...}

export function getSettings(workspace: ProjectWorkspace, options?: Options): Promise<JestSettings> {...}

Is everyone fine with that?

SimenB · 2019-07-18T07:07:34Z

You can pull ProjectConfiguration from @jest/types if you want: https://github.com/facebook/jest/blob/bd76829f66c5c0f3c6907b80010f19893cb0fc8c/packages/jest-types/src/Config.ts#L367-L421

Doesn't really make a difference of course, just throwing it out there 🙂 Might not be worth the dep (as it's just strings or arrays of strings)

stephtr · 2019-07-18T07:52:45Z

Doesn't really make a difference of course, just throwing it out there 🙂 Might not be worth the dep (as it's just strings or arrays of strings)

That definitely makes sense.
Since I'm not that experienced with Flow.js and flow status doesn't check it, is this import statement correct?

import type {Config as JestConfig} from '@jest/types';

type JestSettings = {
  jestVersionMajor: number,
  configs: JestConfig.ProjectConfig[],
};

SimenB · 2019-07-18T07:54:09Z

Oh, is this written in Flow? Then I apologize - we only export TS types... I'd forgotten this was extracted before we migrated to TS 😬

stephtr · 2019-07-18T08:20:07Z

It's fine, it is still useful for the index.d.ts file.
I guess we should also migrate jest-editor-support to TS earlier or later.

seanpoulter · 2019-07-18T18:51:24Z

Oh, is this written in Flow? Then I apologize - we only export TS types... I'd forgotten this was extracted before we migrated to TS 😬

Was all of that conversion done manually @SimenB?

SimenB · 2019-07-18T22:31:44Z

Yeah - it was pretty straight forward to just rename the file, replace all mixed and * with unknown and hammer at the squiggles until they were gone. I don't know the status of bcherny/flow-to-typescript#12, but it looked very promising last I looked at it.

(I'll try to write a blog post about the migration in the next couple of weeks - my vacation starts tomorrow and the weather in Oslo is gonna be pretty bad, so I'll have some time in the next few day to get a good start)

stephtr · 2019-07-18T23:24:23Z

Nevertheless have a nice vacation!

I just updated the tests, now my PR would be complete.
By the way, I never thought I would see something like that in real life 😉

const makeBuffer = (content: string) => {
  // Buffer.from is not supported in < Node 5.10
  if (typeof Buffer.from === 'function') {
    return Buffer.from(content);
  }
  return Buffer.from(content);
};

connectdotz · 2019-07-22T08:26:16Z

@seanpoulter since you already started the review, wanna finish it?

We have a pre-release 26-beta, once this PR is merged, we could cut an official v26.0.0

seanpoulter · 2019-07-23T06:10:01Z

@seanpoulter since you already started the review, wanna finish it?

Sorry @connectdotz, I didn't start a review. I stopped at pointing out the code smell. If anyone else has the capacity please feel free to do the code review. I'm worried you might be waiting on me to have enough spare time to relearn what's going on in this repo.

connectdotz

looks good to me, thanks @stephtr 👍

connectdotz · 2019-07-28T15:54:50Z

@stephtr just discovered this in vscode-jest when linking with the merged jest-editor-support:

$ yarn test
...
 FAIL  tests/diagnostics.test.ts
  ● Test suite failed to run

    TypeError: Cannot read property 'length' of undefined

      at Object.<anonymous> (../jest-editor-support/node_modules/core-js/modules/es6.symbol.js:170:71)

Apparently, all tests depends on jest-editor-support failed. Is this babel related?

connectdotz · 2019-07-29T10:40:16Z

@stephtr do you prefer we revert the merge until we figure out what caused this new error, or to patch it in a separate PR?

stephtr · 2019-07-29T13:57:47Z

I'll take a look at it in the evening. I initially thought that settings wouldn't affect the vscode plug in...

…

________________________________ From: ConnectDotz <notifications@github.com> Sent: Monday, July 29, 2019 12:40:17 PM To: jest-community/jest-editor-support <jest-editor-support@noreply.github.com> Cc: Stephan Troyer <stephantroyer@live.at>; Mention <mention@noreply.github.com> Subject: Re: [jest-community/jest-editor-support] Replace `Settings` class by `getSettings` function (#14) @stephtr<https://github.com/stephtr> do you prefer we revert the merge until we figure out what caused this new error, or to patch it in a separate PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#14?email_source=notifications&email_token=ABL7IWDR6ILX7R7RLCAWIYLQB3CJDA5CNFSM4IDSBZC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3AKCXI#issuecomment-515940701>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABL7IWC4LEDXIEAS3JHU5JLQB3CJDANCNFSM4IDSBZCQ>.

stephtr · 2019-08-01T11:57:50Z

Unfortunately I still haven't found the underlying issue even though I tried a few things. My guess however still is, that it is rather an issue with a dependency update between the last two versions than related to this PR. Today I try to find the culprit commit.

connectdotz · 2019-10-06T14:08:00Z

@stephtr I tried linking vscode-jest with jest-editor-support again today, still getting errors:

 FAIL  tests/JestProcessManagement/JestProcess.test.ts
  ● Test suite failed to run

    TypeError: Cannot read property 'length' of undefined

      at Object.<anonymous> (../jest-editor-support/node_modules/core-js/modules/es6.symbol.js:170:71)

any thought on how to fix it?

stephtr · 2019-10-06T15:13:19Z

I also gave it another try today morning. However so far I wasn't successful. I tried previous commits but found no one which wasn't throwing some exceptions. Updating babel also didn't help.

any thought on how to fix it?

Currently I don't even understand the underlying problem. I'll still play around with it a bit today, but unfortunately my hope isn't that high.

Update: I got only so far, that my error message currently is:

TypeError: Cannot destructure property `formatters` of 'undefined' or 'null'.
      at Object.<anonymous> (../jest-editor-support/node_modules/debug/src/node.js:238:7)

I have the feeling that this is coming from vscode-jest test unintentionally mocking an internal function within node_modules. I will try previous versions of vscode-jest when I will have a more stable internet connection later today.

seanpoulter · 2019-10-06T17:58:27Z

I might have a chance to investigate tonight. Are these the right steps to reproduce it? Feel free to edit this message as required.

Steps to reproduce

Clone vscode-jest and jest-editor-support
Use the local jest-editor-support in the vscode-jest repo
Run the tests

Expected behaviour

Unit tests for jest-editor-support pass
Unit tests for vscode-jest pass

Actual behaviour

The two different errors above

stephtr · 2019-10-06T20:08:44Z

Exactly. It always boils down to the fact, that some function result within node_modules is undefined even though it shouldn't be the case. In my cases the reason was always that some function inadvertently got mocked (I assume by Jest) after a few tests succeeded. Mocked the functions just return undefined, which leads to the observed exceptions.

connectdotz · 2019-10-06T20:17:49Z

@stephtr you can try using jestSetup.js to force it unmock...

stephtr · 2019-10-06T20:47:05Z

@stephtr you can try using jestSetup.js to force it unmock...

Wow thanks, I now at least see the smoking gun. The entries of jestSetup.js don't work if the dependencies happen to be located within node_modules of another dependency. I therefore had to add a few lines of jest.unmock('jest-editor-support/node_modules/... (also for some new modules).
But why do those modules get mocked in the first place and how to solve that?

connectdotz · 2019-10-06T21:05:13Z

vscode-jest has automock on, and I believe jest skip automock on its "internal" packages automatically so we never did see this problem when jest-editor-support was part of its repo...

Perhaps we should consider turning off automock in vscode-jest, but I suspect that could trigger quite a few tests changes...

stephtr · 2019-10-06T21:25:41Z

Perhaps we should consider turning off automock in vscode-jest, but I suspect that could trigger quite a few tests changes...

Maybe that's the best way, even though it would require quite some work on manual mocking (without automock the tests result in many pages of error messages).

connectdotz · 2019-10-07T13:57:48Z

ok, I found a cheap fix with unmockedModulePathPatterns...

unmockedModulePathPatterns: ['jest-editor-support/node_modules'],

After adding this to jest.config.js in vscode-jest, all tests passed... and we don't even need the jestSetup.js any more

stephtr · 2019-10-07T17:55:11Z

Wow, that really sounds perfect!

remove default values from Settings

f1ac74d

stephtr added the breaking change label Jul 14, 2019

stephtr requested a review from connectdotz July 14, 2019 21:57

seanpoulter reviewed Jul 14, 2019

View reviewed changes

yarn.lock Show resolved Hide resolved

rossknudsen mentioned this pull request Jul 15, 2019

Bumped jest-config version to fix vulnerabilities #12

Closed

remodel Settings

e5fde3b

stephtr added 2 commits July 18, 2019 08:52

update changelog

fa83c18

update comment

f50e634

stephtr changed the title ~~Remove default configuration values from Settings~~ Replace Settings class by getSettings function Jul 18, 2019

add @jest/types dependency, fix small bugs

e453384

Revert use of @jest/types in Settings.js

ef65d70

add comment about Config class

198d743

stephtr added 5 commits July 19, 2019 00:58

update tests

aad6f61

fix flow error

1616e8c

typescript definition: export correct function

8026e92

run prettier

bb5e9b4

fix eslint issues

cb1e1c6

update test

d5b4e4e

connectdotz approved these changes Jul 28, 2019

View reviewed changes

connectdotz merged commit 690aa00 into jest-community:master Jul 28, 2019

connectdotz mentioned this pull request Oct 7, 2019

fix jest-editor-support tests breakage jest-community/vscode-jest#508

Merged

Replace Settings class by getSettings function #14

Replace Settings class by getSettings function #14

Uh oh!

Conversation

stephtr commented Jul 14, 2019

Uh oh!

Uh oh!

seanpoulter commented Jul 14, 2019

Uh oh!

stephtr commented Jul 15, 2019

Uh oh!

rossknudsen commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanpoulter commented Jul 15, 2019

Uh oh!

connectdotz commented Jul 15, 2019

Uh oh!

stephtr commented Jul 18, 2019

Uh oh!

SimenB commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephtr commented Jul 18, 2019

Uh oh!

SimenB commented Jul 18, 2019

Uh oh!

stephtr commented Jul 18, 2019

Uh oh!

seanpoulter commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephtr commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connectdotz commented Jul 22, 2019

Uh oh!

seanpoulter commented Jul 23, 2019

Uh oh!

connectdotz left a comment

Choose a reason for hiding this comment

Uh oh!

connectdotz commented Jul 28, 2019

Uh oh!

connectdotz commented Jul 29, 2019

Uh oh!

stephtr commented Jul 29, 2019 via email

Uh oh!

stephtr commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connectdotz commented Oct 6, 2019

Uh oh!

stephtr commented Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanpoulter commented Oct 6, 2019

Uh oh!

stephtr commented Oct 6, 2019

Uh oh!

connectdotz commented Oct 6, 2019

Uh oh!

stephtr commented Oct 6, 2019

Uh oh!

connectdotz commented Oct 6, 2019

Uh oh!

stephtr commented Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connectdotz commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephtr commented Oct 7, 2019

Uh oh!

Uh oh!

Replace `Settings` class by `getSettings` function #14

Replace `Settings` class by `getSettings` function #14

rossknudsen commented Jul 15, 2019 •

edited

Loading

SimenB commented Jul 18, 2019 •

edited

Loading

seanpoulter commented Jul 18, 2019 •

edited

Loading

SimenB commented Jul 18, 2019 •

edited

Loading

stephtr commented Jul 18, 2019 •

edited

Loading

stephtr commented Aug 1, 2019 •

edited

Loading

stephtr commented Oct 6, 2019 •

edited

Loading

stephtr commented Oct 6, 2019 •

edited

Loading

connectdotz commented Oct 7, 2019 •

edited

Loading