Add more unit tests for PyButton and BaseEvalElement by FabioRosado · Pull Request #711 · pyscript/pyscript

FabioRosado · 2022-08-23T00:56:02Z

This PR is part of the effort to increase test coverage (#679). Two new test files were added:

one for PyButton
one for BaseEvalElement

I wonder if this is the right approach to testing each component and perhaps some internals. Many of these tests are just asserting default behaviours and checking that things changed as we expect when calling functions.

Please let me know if you would prefer to test these differently or if you think it's good then I'll keep writing tests for the other files 😄

marimeireles

This is great effort!
Thank you so much @FabioRosado :)
My comment on the test with the error is because I'm thinking well, mostly about maintainability... Should we create tests that we know it fails?
@antocuni will probably have an input here as he introduced this discussion on our previous PR.
But if anyone else wants to chime in, please!

marimeireles · 2022-08-25T12:40:40Z

+    it('BaseEvalElement instantiates correctly', async () => {
+        expect(instance).toBeInstanceOf(BaseEvalElement);
+    });
+
+    // TODO: This test is a bit silly, but the idea is to keep it here until we fix this
+    // issue
+    it("should fail with: Cannot use 'in' operator to search for 'runPythonAsync' in undefined", async () => {
+        let thrownError;
+        let expectedTypeError = new TypeError("Cannot use 'in' operator to search for 'runPythonAsync' in undefined");
+        try {
+            instance.runAfterRuntimeInitialized(async () => {
+                return;
+            });
+        } catch (error) {
+            thrownError = error;
+        }
+        expect(thrownError).toEqual(expectedTypeError);
+    });


I'm wondering there's no other way to test this functionality? Or to ignore this error?

That's a great question(s)!

We could mock runAfterRuntimeInitialized similar to what I've done in pybutton.test.ts so this exception won't impact the tests.

This test was added mostly because I was poking at the exception and trying to figure out what's happening and thought it could be good to include in the PR so that when we fix the issue, his test will fail and we can remove it. Maybe that's a bad idea? 😄

Happy to remove it if you think is best

I think it might be the best approach for now. But we can get back to it once Antonio is back on vacation and I hear what he has to say =)

This test was added mostly because I was poking at the exception and trying to figure out what's happening and thought it could be good to include in the PR so that when we fix the issue, his test will fail and we can remove it. Maybe that's a bad idea?

I think it's a very good idea! +1

FabioRosado · 2022-08-25T13:31:51Z

Thank you, if this seems like a good approach I'll start adding more tests to cover other parts 😄

marimeireles · 2022-08-25T18:16:09Z

Yeah sure! This is a great approach.
Thanks Fabio. =)

marimeireles · 2022-08-25T21:28:54Z

Also, seems like there are some small issues with the tests.
I'm wondering if this 4 import { pyodideLoaded } from '../../src/stores'; error is the same one as we were debugging together the other day @madhur-tandon, with your tests? (That you had to create a mockup, etc...)

FabioRosado · 2022-08-25T21:35:52Z

I think this might be happening because I haven't merged main into this branch yet 🤔

Let me try doing that and run the tests again just to see

madhur-tandon · 2022-08-25T21:38:03Z

Hi @FabioRosado you might wanna rebase on main

FabioRosado · 2022-08-25T22:31:21Z

Thanks Madhur and Mariana! I've rebased main into the branch and fixed the imports, I've also marked the integration test test_repl as xfail here since it seems this test has been failing very often in CI

marimeireles · 2022-08-26T09:03:46Z

Thanks for the work Fabio! ✨
Will review it now 😊

marimeireles · 2022-08-26T12:25:21Z

I was trying to reproduce the test_repl error locally but can't.
Which makes me wonder if there's something related to our CI? Like do we need more time to run these tests?
Other than that! 🎉
Thank you Fabio! :)

FabioRosado · 2022-08-26T13:33:21Z

Thank you Mariana!
Yeah it seems to be flaky at times, I've installed pytest-repeat and did 20 runs of the reply test and it failed 4 times, I'm wondering if sometimes the repl doesn't return quick enough

antocuni

Sorry for the late review! @FabioRosado thank you for this PR, I'm always happy when I see new tests :)

To answer your question:

I wonder if this is the right approach to testing each component and perhaps some internals.
[cut]
Please let me know if you would prefer to test these differently

JS unit tests are good, and our current situation is so bad that every progress in this area is very welcome.
Personally, I think that integration tests are more important, though.

In my view, unit tests are more an internal tool which is useful for us to ensure the correctness and quality of our code.

On the other hand, integration tests represent our contract with the end users: they declare (and check!) what happens when you use pyscript in your HTML page and that when we refactor the code we don't accidentally break legitimate usage of it. For every user-visible functionality of pyscript, we surely want to have at least one integration test which checks that it keep working as expected.

In the long run both are important, but if I have to choose, in the short term I'd probably prefer to increase the coverage of integration tests. But this is just my personal opinion which might or might not be shared by the rest of the team

antocuni · 2022-08-31T13:49:55Z

+        }
+        expect(thrownError).toEqual(expectedTypeError);
+    });
+


Disclaimer: this is literally the first time ever which I see a Javascript/Jest unit test so I don't really know about the idioms. If I understand correctly, this is basically the equivalent of pytest.raises, isn't it?
Is it there a more direct way to do it? A quick googling tells me about expect().toThrow() but I don't really know whether it applies here.

Also, small nitpick: it would have been nicer to put the issue number in the comment, so that it will be easier to understand why the test fails once the issue it's fixed.

Yeah my idea was to write a test similar to pytest.raises 😄

Oh I didn't knew about the toThrow check, I'm still getting used writing js/jest tests haha
That's a good point that didn't occur to me. I'm planning to write more tests over the weekend so I'll make sure to add the issue number on this one 😄

antocuni · 2022-08-31T13:52:54Z

+    it('BaseEvalElement instantiates correctly', async () => {
+        expect(instance).toBeInstanceOf(BaseEvalElement);
+    });
+
+    // TODO: This test is a bit silly, but the idea is to keep it here until we fix this
+    // issue
+    it("should fail with: Cannot use 'in' operator to search for 'runPythonAsync' in undefined", async () => {
+        let thrownError;
+        let expectedTypeError = new TypeError("Cannot use 'in' operator to search for 'runPythonAsync' in undefined");
+        try {
+            instance.runAfterRuntimeInitialized(async () => {
+                return;
+            });
+        } catch (error) {
+            thrownError = error;
+        }
+        expect(thrownError).toEqual(expectedTypeError);
+    });


This test was added mostly because I was poking at the exception and trying to figure out what's happening and thought it could be good to include in the PR so that when we fix the issue, his test will fail and we can remove it. Maybe that's a bad idea?

I think it's a very good idea! +1

FabioRosado · 2022-08-31T14:11:25Z

Thank you for the review Antonio, that makes sense, I'll keep adding both integration tests and unit ones too now that I know this is a good approach 😄

marimeireles reviewed Aug 25, 2022

View reviewed changes

FabioRosado added 2 commits August 25, 2022 22:39

Add more unit tests

7e10576

Rebase main and fix failing tests

5234b0d

FabioRosado force-pushed the fr/jest branch from 492e144 to 5234b0d Compare August 25, 2022 22:02

FabioRosado added 2 commits August 25, 2022 23:20

xfail flaky test

6281227

Fix import

1248054

FabioRosado mentioned this pull request Aug 25, 2022

The integration test 'test_repl' is flaky #726

Closed

3 tasks

marimeireles approved these changes Aug 26, 2022

View reviewed changes

marimeireles merged commit 9de1545 into pyscript:main Aug 26, 2022

antocuni reviewed Aug 31, 2022

View reviewed changes

FabioRosado deleted the fr/jest branch August 31, 2022 14:11

Conversation

FabioRosado commented Aug 23, 2022

Uh oh!

marimeireles left a comment

Choose a reason for hiding this comment

Uh oh!

marimeireles Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

FabioRosado Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marimeireles Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

antocuni Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FabioRosado commented Aug 25, 2022

Uh oh!

marimeireles commented Aug 25, 2022

Uh oh!

marimeireles commented Aug 25, 2022

Uh oh!

FabioRosado commented Aug 25, 2022

Uh oh!

madhur-tandon commented Aug 25, 2022

Uh oh!

FabioRosado commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marimeireles commented Aug 26, 2022

Uh oh!

marimeireles commented Aug 26, 2022

Uh oh!

FabioRosado commented Aug 26, 2022

Uh oh!

antocuni left a comment

Choose a reason for hiding this comment

Uh oh!

antocuni Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

FabioRosado Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

antocuni Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

FabioRosado commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FabioRosado Aug 25, 2022 •

edited

Loading

FabioRosado commented Aug 25, 2022 •

edited

Loading