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

Switch IteratorTest to use TF combinations #32599

Merged
merged 2 commits into from
Sep 20, 2019
Merged

Switch IteratorTest to use TF combinations #32599

merged 2 commits into from
Sep 20, 2019

Conversation

feihugis
Copy link
Member

This PR switch IteratorTest to use TF combinations.

@tensorflow-bot tensorflow-bot bot added the size:M CL Change Size: Medium label Sep 18, 2019
@feihugis feihugis force-pushed the TF_DS_Combination branch 2 times, most recently from fa2fe76 to 4a9296b Compare September 18, 2019 00:12
@gbaned gbaned self-assigned this Sep 18, 2019
@gbaned gbaned requested a review from jsimsa September 18, 2019 03:37
jsimsa
jsimsa reviewed Sep 18, 2019
View reviewed changes
tensorflow/python/data/kernel_tests/iterator_test.py
"b": ([1], [])
}),
)
@combinations.generate(combinations.times(
Copy link
Contributor

Choose a reason for hiding this comment

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

As a rule of thumb, we have agreed on the tf.data team that going forward we should not used parametrized tests that require lambda parameters and instead create separate unit tests. Could you please split this tests into separate tests, one for each test case? Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do it.

@feihugis
Copy link
Member Author

@jsimsa testIteratorStructure() is split into three test functions to avoid the use of lambda parameter. Could you please take a look at the change(7268f8d)?

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 18, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 18, 2019
@feihugis feihugis added the kokoro:force-run Tests on submitted change label Sep 18, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 18, 2019
@feihugis feihugis added the kokoro:force-run Tests on submitted change label Sep 19, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 19, 2019
@feihugis
Copy link
Member Author

@gbaned An error happened while migrating the change for the internal check. Could you please help re-trigger the internal check? Thanks!

@gbaned gbaned added the kokoro:force-run Tests on submitted change label Sep 20, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 20, 2019
tensorflow-copybara pushed a commit that referenced this pull request Sep 20, 2019
@tensorflower-gardener
Merge pull request #32599 from feihugis:TF_DS_Combination
9556134 
PiperOrigin-RevId: 270286442
@tensorflow-copybara tensorflow-copybara merged commit 7268f8d into tensorflow:master Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimsa jsimsa jsimsa approved these changes

Assignees

@gbaned gbaned

Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@feihugis @jsimsa @googlebot @kokoro-team @tensorflow-copybara @gbaned