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

Tech/update jest, babel and adapt mocks #1097

Merged
merged 10 commits into from
Jul 14, 2020
Merged

Tech/update jest, babel and adapt mocks #1097

merged 10 commits into from
Jul 14, 2020

Conversation

NearW
Copy link
Contributor

@NearW NearW commented Jul 13, 2020

Update Jest, Babel and adapt broken tests due to jest mock change

Description

  • Jest > 24 requires Babel 7 to work, I updated both
  • Jest > 22 breaks the mocking behaviour, meaning we can't use our old mock syntax. I adapted these mocks
  • Removed ng-annotate-loader since we don't use ng-annotations

I hope that with updating Babel, I can rework our compiling and transpiling flow and fix my issues in #992

Definition of Done

A task/pull request will not be considered to be complete until all these items can be checked off.

  • All requirements mentioned in the issue are implemented
  • Does match the Code of Conduct and the Contribution file
  • Task has its own GitHub issue (something it is solving)
    • Issue number is included in the commit messages for traceability
  • Update the README.md with any changes/additions made
  • Update the CHANGELOG.md with any changes/additions made
  • Enough test coverage to ensure that uncovered changes do not break functionality
  • All tests pass
  • Descriptive pull request text, answering:
    • What problem/issue are you fixing?
    • What does this PR implement and how?
  • Assign your PR to someone for a code review
    • This person will be contacted first if a bug is introduced into master
  • Manual testing did not fail

@NearW NearW added pr-visualization Issues that touch the visualization pr(oject) which means web and desktop features. tech For technical stories without user impact (=refactoring stories). labels Jul 13, 2020
@NearW NearW changed the title Tech/update jest Tech/update jest, babel and adapt mocks Jul 13, 2020
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Awesome work! Just left a few small comments.

}
})()
codeMapPreRenderService.getRenderFileMeta = jest.fn().mockReturnValue({ fileName: "my_fileName" })
attributeSideBarController["codeMapPreRenderService"] = codeMapPreRenderService
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to add a linter rule to use the dot notation in the future (if you agree, in a separate PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accessing private fields or functions is quite useful when writing tests or setting up a test.
We could enable that rule for our production code for sure, but tests shouldn't be included.

Copy link
Member

Choose a reason for hiding this comment

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

Using the dot notation is independent from accessing private fields :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How? When I want to access a private field, I have to use class["private_field"]? instead of class.private_field

Copy link
Member

@BridgeAR BridgeAR Jul 13, 2020

Choose a reason for hiding this comment

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

TIL. I was not aware that TypeScript used this specific notation to access private fields. IMO it's a bad practice to access any private field. I do not want to go into that discussion here though. I would actively mark access like these (by adding a comment) that it's a private properly that it's intentionally accessed and that it's the TypeScript way to do so.

Calling something a private field that is still accessible is somewhat weird. The reason that it's working like that was AFAIK that it was difficult to create actual private members when TypeScript came up. That changed in the meanwhile and now JS has actual private fields that do not allow any access from outside.

I personally always test internals by either: using the dependency injection pattern or by just plainly checking the overall output of a higher level API that should only work if the internal path is hit. If the coverage does not show the expected lines to be covered, something is fishy.


To get back to the actual comment: for now it would be possible to exclude tests for such a rule. On the long term, I'd try to refactor the code to the dependency injection pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use the typescript-es-lint rule. It has an option to allow private class member access like this.

I'm not sure if you can allow/disallow individual rules. I think you can just define a pattern that applies to all rules.

Copy link
Member

Choose a reason for hiding this comment

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

It is possible to enable specific rules for specific files by using overrides. It should be sufficient to just use the "native" rule.

visualization/babel.config.json Show resolved Hide resolved
visualization/conf/webpack.loaders.js Show resolved Hide resolved
visualization/package.json Outdated Show resolved Hide resolved
BridgeAR
BridgeAR previously approved these changes Jul 14, 2020
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM! Just left one question and the promise part could be shorter.

visualization/package.json Show resolved Hide resolved
visualization/app/codeCharta/util/urlExtractor.spec.ts Outdated Show resolved Hide resolved
visualization/app/codeCharta/util/urlExtractor.spec.ts Outdated Show resolved Hide resolved
@NearW NearW merged commit 98f921c into master Jul 14, 2020
@NearW NearW deleted the tech/update-jest branch July 14, 2020 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-visualization Issues that touch the visualization pr(oject) which means web and desktop features. tech For technical stories without user impact (=refactoring stories).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants