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.

Sign up for GitHub

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

Jump to bottom

feat: use new SDK standard tests framework #5

Closed
wants to merge 10 commits into from
Closed

feat: use new SDK standard tests framework #5

wants to merge 10 commits into from

Conversation

kgpayne
Copy link

@kgpayne kgpayne commented Jan 26, 2023

No description provided.

@kgpayne kgpayne mentioned this pull request Jan 26, 2023
Open
8 tasks
@kgpayne kgpayne force-pushed the kgpayne/test_standard_sdk_tests branch from c63194e to a2827eb Compare January 26, 2023 12:02
@kgpayne
Copy link
Author

kgpayne commented Jan 26, 2023

@MeltanoLabs/engineering any ideas why mypy might be failing here 🤔 The Property.default type hint is default: _JsonValue | None = None so not sure why its not happy:

_JsonValue: TypeAlias = Union[
    str,
    int,
    float,
    bool,
    list,
    dict,
    None,
]

Maybe that last None should be a Type[None] 🤷‍♂️

@edgarrmondragon
Copy link
Member

edgarrmondragon commented Jan 26, 2023 •
edited
Loading

@MeltanoLabs/engineering any ideas why mypy might be failing here 🤔 The Property.default type hint is default: _JsonValue | None = None so not sure why its not happy

@kgpayne Try updating mypy that was it, it seems

aaronsteers
aaronsteers reviewed Jan 26, 2023
View reviewed changes
target_csv/tests/test_core.py Outdated
Comment on lines 17 to 20
tests = get_standard_target_tests(
TargetCSV,
config=SAMPLE_CONFIG,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@kgpayne - can you confirm if this is backwards compatible? What would happen if users bump the SDK without changing this test strategy? Ideally we would not break existing implementations - either we run all new tests, or run some subset - but we definitely would not want all taps and targets to have their tests broken with their next SDK version bump.

Copy link
Contributor

Choose a reason for hiding this comment

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

I decided this was easier to just test by reverting the dropped tests:

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Legacy tests still pass! 🎉

@aaronsteers aaronsteers mentioned this pull request Jan 26, 2023
Merged
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@edgarrmondragon
Copy link
Member

Superseded by changes in #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronsteers aaronsteers aaronsteers left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kgpayne @edgarrmondragon @aaronsteers