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

Fix tests and lint #211

Merged
merged 8 commits into from May 2, 2022
Merged

Conversation

nwalters512
Copy link
Contributor

@nwalters512 nwalters512 commented May 2, 2022

This fixes the failing test called out in #174 (comment). It also fixes a few more failing tests. The entire test suite now passes!

I also took the opportunity to remove some commented-out assertions and remove a few unnecessary bits that had // eslint-disable... comments.

I would strongly encourage the maintainers to set up CI to run on pull requests, not just on master, so that errors can be caught quickly and resolved before changes are merged!

expect(successResult.resultType).toBe("success");
expect(successResult.absoluteBaseUrl).toBe(join("/baz", "src"));
});

it("should show an error message when baseUrl is missing", () => {
it("should tolerate a missing baseUrl", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this test was broken by #208.

tsConfigPath: "/baz/tsconfig.json",
baseUrl: "/baz",
paths: {},
}),
});

const successResult = result as ConfigLoaderSuccessResult;
assert.equal(successResult.absoluteBaseUrl, "/baz");
expect(successResult.absoluteBaseUrl).toEqual("/baz");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the failing test from #174.

expect(result).toEqual([
{
pattern: "longest/pre/fix/*",
paths: [join("/absolute", "base", "url", "foo2", "bar")],
},
{
pattern: "pre/fix/*",
paths: [join("/absolute", "base", "url", "foo3")],
paths: [join("/foo3")],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these test cases were broken by #184. I updated them to have a sampling of both relative and absolute paths.

@nwalters512 nwalters512 marked this pull request as ready for review May 2, 2022 18:56
@jonaskello
Copy link
Member

@nwalters512 I was trying to get the tests running in CI for the PR. Could you try to merge master into this and push again?

@nwalters512
Copy link
Contributor Author

@jonaskello I believe you're missing configuration from .github/workflows/ci.yml. You need on: [push, pull_request] instead of just on: [push]. The push event only handles pushes to this repo, not pushes to forks, which will be the workflow for external contributors. See https://github.com/scinos/yarn-deduplicate/blob/1d0906c46895ffc4a2f22ef762ed6e8474327944/.github/workflows/node.js.yml#L6 for an example.

@jonaskello
Copy link
Member

@nwalters512 Ah, thanks, that may be it! It was running for the PR where I changed the workflow but I guess counts as just push. I updated master according to your example, could you try to merge it into this PR again?

@nwalters512
Copy link
Contributor Author

@jonaskello looks like we're good now!

@codecov
Copy link

codecov bot commented May 2, 2022

Codecov Report

Merging #211 (4a9240e) into master (98bbcdc) will decrease coverage by 2.49%.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
- Coverage   70.26%   67.76%   -2.50%     
==========================================
  Files          10        9       -1     
  Lines         306      304       -2     
  Branches       92       93       +1     
==========================================
- Hits          215      206       -9     
- Misses         85       92       +7     
  Partials        6        6              
Impacted Files Coverage Δ
src/register.ts 0.00% <0.00%> (ø)
src/config-loader.ts 83.33% <66.66%> (-3.04%) ⬇️
src/mapping-entry.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e4452c...4a9240e. Read the comment docs.

@jonaskello
Copy link
Member

@nwalters512 Perfect, thanks for the hint :-)

@jonaskello jonaskello merged commit bad40f2 into dividab:master May 2, 2022
@nwalters512 nwalters512 deleted the fix-tests-and-lint branch May 3, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants