amp-toolbox icon indicating copy to clipboard operation
amp-toolbox copied to clipboard

Migrate toolbox-linter to jest

Open sebastianbenz opened this issue 4 years ago • 2 comments

The custom setup is very hard to maintain and not very portable.

@ithinkihaveacat do you see reasons why this wouldn't work?

This also addresses #668

sebastianbenz avatar Mar 31 '20 19:03 sebastianbenz

jest should work!

Although, I don't think there's any particular portability issues with the code itself. The "test" script in linter/package.json is not portable, but that requires less effort to fix than migrating to jest.

How much work this is depends on how much should be migrated to jest. The existing tests have this structure:

withFixture("thumbnails2", () =>
  assertMatch(
    `${StoryMetadataThumbnailsAreOk.name} - publisher-logo-src missing`,
    runNetworkTest(
      StoryMetadataThumbnailsAreOk,
      "https://regular-biology.glitch.me/"
    ),
    "publisher-logo-src"
  )
);

The three "custom" functions here are:

  • withFixture() - this uses nock to set up HTTP fixtures. I think something like nock that automates the recording and reply of HTTP requests really needs to be used here, it's not feasible to wire everything up by hand. (And in many cases, the network requests happen inside other libraries like probe-image-size.) I don't know if nock can be used with jest.
  • assertMatch() - there's a few different assert*() functions. Should be mostly straightforward to replace with jest equivalents.
  • runNetworkTest() - some initialization boilerplate that sets things up and then runs the test. (There's also a runLocalTest().) The bit that does the initialization should probably be extracted.

So after these changes, the tests would look something like:

withFixture("thumbnails2", () =>
  test(`${StoryMetadataThumbnailsAreOk.name} - publisher-logo-src missing`, () => {
    const context = getNetworkContext("https://regular-biology.glitch.me/");
    const rule = new StoryMetadataThumbnailsAreOk();
    expect(rule.run(context)).toBe("publisher-logo-src");
  });
);

Is this about you're expecting?

(Also, I unfortunately don't have time to work on this now.)

ithinkihaveacat avatar Apr 01 '20 12:04 ithinkihaveacat

I simply want to avoid having to maintain two completely different test setups.

sebastianbenz avatar Apr 01 '20 14:04 sebastianbenz