Skip to content
This repository has been archived by the owner on Oct 30, 2022. It is now read-only.

Fix small oversights #102

Merged
merged 4 commits into from
Jul 24, 2019
Merged

Fix small oversights #102

merged 4 commits into from
Jul 24, 2019

Conversation

idantene
Copy link
Contributor

  • Forgot to update states returned from e.g. responseCreatorFactory to use the new types from Adds typing for state store #99
  • Updates matcher to replace server path prefix fully and not by partial match. Error rose when trying to set state for path /api.test when the server prefix is /api. Instead of replacing /api with the empty string, replaces /api/ with / instead.

@idantene idantene requested review from mikesol and ksaaskil July 24, 2019 08:46
@@ -63,58 +63,60 @@ describe("Fluent API and Service instantiation tests", () => {

it("Store with existing path does not throw", () => {
const store = stateFacadeFactory(PetStoreWithPseudoResponses);
store.petstore(); // Should pass
store.petstore({}); // Should pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, why didn't this throw an error before if store.petstore() does not make sense? 🤔

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 updated it just to fix the type hinting, behind the scenes an empty object is treated the same as undefined object (no-op). Not sure if it should throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't understand what there was to "fix" if users can anyway do either 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

New types mean vscode underlines such mistakes (even though both are acceptable) -
image

Alternatively ofc I could include 0 arguments as valid, if you feel that's more correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, just wondering why tsc compiled if it's wrong, maybe we don't even do type checking for the test files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, we use npm ci in CI, which implicitly calls postinstall, which calls compile 😇

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, if compiling test and source code happen in the same step I sure hope lerna hows to keep the build clean!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's taken care of by tsc --build tsconfig.test.json --clean, which removes the compiled test files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'm still so suspicious that I'll probably have to test adding a type check error in the test files and see what happens myself but this PR is of course very good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I just noticed I forgot the update to package.json, so that clarifies all this confusion hopefully. 🤦‍♂️ My bad.

@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #102 into dev will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev     #102      +/-   ##
=========================================
+ Coverage   83.8%   84.15%   +0.35%     
=========================================
  Files         32       32              
  Lines       1142     1142              
  Branches     259      260       +1     
=========================================
+ Hits         957      961       +4     
+ Misses       185      181       -4
Impacted Files Coverage Δ
packages/unmock-core/src/generator.ts 20.61% <100%> (ø) ⬆️
packages/unmock-core/src/service/matcher.ts 94.11% <100%> (+2.35%) ⬆️
packages/unmock-core/src/service/index.ts 87.17% <0%> (+5.12%) ⬆️

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 ba6a78f...b97867f. Read the comment docs.

@idantene idantene merged commit 30083e5 into dev Jul 24, 2019
@idantene idantene deleted the fix-small-oversights branch July 24, 2019 12:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants