balazsbotond / urlcat

A URL builder library for JavaScript.
https://urlcat.org
MIT License
1.82k stars 57 forks source link

Configure Travis CI to run tests on Deno #43

Closed balazsbotond closed 3 years ago

balazsbotond commented 3 years ago

urlcat currently works with Deno - I know because I've manually tested it before the latest release. This is obviously very boring, easy to forget and error-prone, so it would be great if we could run the test suite on Deno too (our build currently runs on Node 10, 11, 12, 13 and 14) after each commit automatically.

If you have experience with Travis CI and Docker, and/or the inclination to dig deep into their documentation and you can deliver a nice, clean solution, please tell us in a comment :)

tcarrio commented 3 years ago

Well, testing CI builds with issue references in the commit really bogs up the issue. My apologies.

@balazsbotond I opened up a PR that introduces this feature. Since running the tests in Deno would require a considerable refactor to the test code, I left that out.

balazsbotond commented 3 years ago

@tcarrio thank you! Could you please describe how you would approach refactoring the tests to run both on Deno and Node so I or someone else could do it later (or you of course, if you feel like it)?

3zk1m0 commented 3 years ago

I Fiddled around yesterday to try this also, and i tried the refactoring by making separate folder (/tests/deno) for denos tests. To refactor, i just used mostly find and replace for to change it to use Denos library. Then i just ignored Denos test folder in jest.config.

My opinion: Its not the cleanest solutions and in future there might be better way to do this. But it works for now 😛

balazsbotond commented 3 years ago

@3zk1m0 maybe we could add a very thin abstraction layer over the APIs of Jest and Deno and write our tests using that layer.

balazsbotond commented 3 years ago

I'm thinking about something like this:

urlcat.ts:

export default {
  name: 'urlcat',
  testCases: [
    {
      name: 'Concatenates the base URL and the path if no params are passed',
      run: assert => {
        const expected = 'http://example.com/path';
        const actual = urlcat('http://example.com', 'path');
        assert.strictEquals(actual, expected);
      }
    },
    // more test cases...
  ]
};

all-tests.ts:

import urlcatSuite from './urlcat';
import substSuite from './subst';
// etc.

export default [
  urlcatSuite,
  substSuite,
];

jest-runner.ts:

import testSuites from './all-tests';

const jestAssert = {
  strictEquals: (e, a) => expect(a).toBe(e),
  // more functions could be added later
};

for (const testSuite of testSuites) {
  describe(testSuite.name, () => {
    for (const testCase of testSuite.testCases) {
      it(testCase.name, () => {
        testCase.run(jestAssert);
      });
    }
  });
}

deno-runner.ts:

import { assert } from "https://deno.land/std@0.72.0/testing/asserts.ts";
import testSuites from './all-tests';

const denoAssert = {
  strictEquals: (e, a) => assert.strictEquals(a, e),
  // more functions could be added later
};

for (const testSuite of testSuites) {
  for (const testCase of testSuite.testCases) {
    Deno.test(`${testSuite.name} / ${testCase.name}`, () => {
      testCase.run(denoAssert);
    });
  }
}

@tcarrio and @3zk1m0 what do you think? Please don't hesitate to tell me if there is a simpler way to do this :)

3zk1m0 commented 3 years ago

@balazsbotond In my opinion that looks lot cleaner. It just needs to be documented so people later understand how it works with out too much effort, even it still is pretty simple to understand.

tcarrio commented 3 years ago

@balazsbotond I think generalizing the test assertions with your own library would work well, with most of the tests assertions in the test suite as it is it should be easy enough to maintain adapters for both Jest and Deno test frameworks.

Your example looks pretty good as well. Are either you or @3zk1m0 looking to implement these? I'm touching up the CI changes now and can move on to this of we can tag team the issue. Let me know :wave:

balazsbotond commented 3 years ago

@tcarrio and @3zk1m0 thanks for your input on this! I was a bit on the fence about this solution but I'm glad you both think it's a good one.

@tcarrio I'd be glad if you could implement the abstraction layer too!

tcarrio commented 3 years ago

The changes recently merged in for #51 break Deno compatibility. Also, it might be a good idea to check the actual minified size of this code now.

Also another note about that change. From the README:

0.8 KB minified and gzipped

That was before the external package was brought in. It's likely larger now and should be updated before the next push to latest

tcarrio commented 3 years ago

I opened up a follow-up task for updating the README to accomodate those merged changes. See #56 for more info.

balazsbotond commented 3 years ago

@all-contributors please add @3zk1m0 for ideas

allcontributors[bot] commented 3 years ago

@balazsbotond

I've put up a pull request to add @3zk1m0! :tada:

balazsbotond commented 3 years ago

I'm closing this issue because the the Travis CI configuration has been merged into the 2.x branch.

I'll open a new issue to continue the discussion about cross-compatible unit testing.