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

test: improve test coverage for DatasetIndexBuilder. #1971

Merged
merged 3 commits into from
Oct 28, 2020
Merged

test: improve test coverage for DatasetIndexBuilder. #1971

merged 3 commits into from
Oct 28, 2020

Conversation

jplaisted
Copy link
Contributor

@jplaisted jplaisted commented Oct 28, 2020

The other index builders need similar improvements, but after #1937 some coverage is better than no coverage.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

The other index builders need similar improvements, but after #1937 some coverage is better than no coverage.
@jplaisted jplaisted requested a review from jywadhwani October 28, 2020 18:35
@mars-lan mars-lan changed the title Improve test coverage for DatasetIndexBuilder. test: improve test coverage for DatasetIndexBuilder. Oct 28, 2020
@jywadhwani
Copy link
Contributor

You may also want to add a test when snapshot contains only InstitutionalMemory aspect?

@jplaisted
Copy link
Contributor Author

jplaisted commented Oct 28, 2020

You may also want to add a test when snapshot contains only InstitutionalMemory aspect?

I thought about this, and the answer is no. Really, we should have some kind of test like that. That all aspects are intentionally handled / not handled. But adding a new aspect means that test will become out of date. So we'd really need some way to automatically generate that test or something, and it would fail as soon as you add a new aspect, requiring you to mark the new aspect as expecting to be handled or not.

Without that automation, I'm not interested in a test like that, no, because it quickly becomes stale and provides no benefit.

We can think about adding that automation later.

@jplaisted jplaisted merged commit b2e73fa into datahub-project:master Oct 28, 2020
@jplaisted jplaisted deleted the tests branch February 26, 2021 22:21
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.

2 participants