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

LIN-161 - unit tests for node transformer. #480

Merged
merged 7 commits into from
Jan 18, 2022

Conversation

lionsardesai
Copy link
Contributor

@lionsardesai lionsardesai commented Jan 13, 2022

Description

Fixes LIN-161

Type of change

Please delete options that are not relevant.

new test cases

How Has This Been Tested?

unit tests added to tests/unit/transformer/*

@lionsardesai lionsardesai marked this pull request as ready for review January 14, 2022 19:24
@lionsardesai lionsardesai changed the title LIN-161 - first round of unit tests for node transformer. LIN-161 - unit tests for node transformer. Jan 14, 2022
Copy link
Contributor

@yifanwu yifanwu left a comment

Choose a reason for hiding this comment

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

Had some questions. The biggest concern is around the extensive use of mocks. Behavioral assertions make low level assumptions about implementation details, making it more time consuming to maintain and are harder to read. I vote for more functional assertions and if we can't do it, we should refactor our code. Happy to continue the discussion on Slack!

lineapy/transformer/source_giver.py Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
setup.py Show resolved Hide resolved
tests/unit/transformer/test_node_transformer.py Outdated Show resolved Hide resolved
tests/unit/transformer/test_node_transformer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@yifanwu yifanwu left a comment

Choose a reason for hiding this comment

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

ship it!

@lionsardesai lionsardesai merged commit 5861298 into main Jan 18, 2022
@lionsardesai lionsardesai deleted the LIN-161-unit-node-transformer branch January 18, 2022 22:27
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