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

Fix AbcTest.testAbstract assertion #47512

Merged
merged 1 commit into from Mar 4, 2021
Merged

Fix AbcTest.testAbstract assertion #47512

merged 1 commit into from Mar 4, 2021

Conversation

park-junha
Copy link
Contributor

Fixes the following test failure:

======================================================================
FAIL: testAbstract (__main__.AbcTest)
testAbstract (__main__.AbcTest)
----------------------------------------------------------------------
TypeError: Can't instantiate abstract class AbstractModule with abstract method __call__

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/buildfarm/default/operations/52eafc82-3509-47a0-ad8a-d565ef4de86b/bazel-out/k8-opt/bin/tensorflow/python/module/module_test.runfiles/org_tensorflow/tensorflow/python/module/module_test.py", line 340, in testAbstract
    AbstractModule()  # pylint: disable=abstract-class-instantiated
AssertionError: "Can't instantiate .* abstract methods" does not match "Can't instantiate abstract class AbstractModule with abstract method __call__"

----------------------------------------------------------------------

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Mar 2, 2021
@google-cla google-cla bot added the cla: yes label Mar 2, 2021
@park-junha park-junha mentioned this pull request Mar 2, 2021
Copy link

@mdanatg mdanatg left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@@ -335,7 +335,7 @@ def testEntersNameScope_concreteFunction(self):
class AbcTest(test_util.TensorFlowTestCase):

def testAbstract(self):
msg = "Can't instantiate .* abstract methods"
msg = "Can't instantiate .* abstract method"
with self.assertRaisesRegex(TypeError, msg):
Copy link

Choose a reason for hiding this comment

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

It would be ok to me to drop the regex altogether - knowing that a TypeError is raised is more than sufficient for this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdanatg FYI - there's a couple instances of assertRaisesRegex in this test suite, I can remove them all if you'd like?

Copy link

Choose a reason for hiding this comment

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

Sounds good. Only those asserts that verify messaged produced by module.py itself should use regexps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdanatg seems like this particular test was the only assert that didn't need a regexp assert, so I went ahead and removed it for that test only.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 2, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 2, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 2, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 2, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 2, 2021
@rthadur
Copy link
Contributor

rthadur commented Mar 2, 2021

@park-junha can you please check sanity build failures ?

@byronyi
Copy link
Contributor

byronyi commented Mar 2, 2021

@park-junha can you please check sanity build failures ?

They look like transient failures. Mind to kick off CI again?

@mdanatg mdanatg added the kokoro:force-run Tests on submitted change label Mar 3, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 3, 2021
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 3, 2021
@copybara-service copybara-service bot merged commit 8d937c4 into tensorflow:master Mar 4, 2021
PR Queue automation moved this from Assigned Reviewer to Merged Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants