search

witness-org / witness

Your fitness witness!
1 stars 0 forks source link

54: Client Integration Tests - [merged] #82

Closed raffaelfoidl closed 2 years ago

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 30, 2021, 15:11

Merges 54-client-integration-tests -> develop

Closes #54

Please see comments in #54 and #56. Flutter runs integration tests only in the emulator if they are located in the integration_test directory - there is nothing to be done here (Flutter ignores the --device flag when running integration tests from the regular test folder).

Since, in the meantime (for the CI), integration tests are intest so that they are executed headless, we can't directly run them in an emulator from IntelliJ. However, especially when writing integration tests, it's useful to see the interface in order to grasp what is happening (and if the correct things are happening). Therefore, I have added the run run_on_device scripts (depending on which shell one prefers) which simply copy the integration tests to integration_test, execute flutter test integration_test and delete integration_test afterwards. This is purely for convenience reasons and the scripts will be removed with #56.

image

Note: The scripts assume that an emulator is already running. Furthermore, they assume no integration_test folder to be present at the beginning. If the tests execute correctly (even if one or more of the tests fail), i.e. the script itself does not crash, the integration_test folder is deleted automatically upon exit. If, however, the script fails or the test execution is aborted (e.g. with CTRL+C), one has to delete the integration_test folder manually.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 30, 2021, 15:13

requested review from @11712616

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 30, 2021, 20:43

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 17:32

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 17:35

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 17:44

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 18:08

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 18:57

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 18:58

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:09

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:14

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:17

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:23

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:29

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:33

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:38

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 1, 2021, 19:43

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 17:38

Commented on client/lib/widgets/authentication/login_card.dart line 46

Maybe rename this variable since it can also be a "sign-up result"? I'm not entirely sure about that because a sign-up operation includes a login, but just from looking at the code, it feels a little bit wrong to name this variable loginResult when it could be the result of a login call or signUp call. Apart from that, I cannot really come up with a better name - maybe something like authenticationResult?

What are your thoughts?

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 17:38

Commented on client/lib/widgets/common/image_provider_facade.dart line 4

"make such code not break" -> "prevent such code from breaking"?

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 17:38

Commented on .gitlab-ci.yml line 1

Is there a particular reason why you put the test stage before the check stage? I would strongly prefer it the other way round. The check stage is way faster and it can result in a lot of frustration if we have to wait for the test stage to succeed only for the check stage to fail afterwards, which would require us to fix the formatting, push the changes and wait for the pipeline to pass again.

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 17:38

Commented on client/test/integration/integration_test.dart line 21

Very cool.

And very well documented, as always.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:36

Commented on client/lib/widgets/authentication/login_card.dart line 46

changed this line in version 17 of the diff

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:36

Commented on client/lib/widgets/common/image_provider_facade.dart line 4

changed this line in version 17 of the diff

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:36

added 4 commits

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:41

added 2 commits

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:45

Commented on client/lib/widgets/authentication/login_card.dart line 46

Yes, it's not so nice. I don't know if I have already discussed it in detail with you, but my plan is to split this widget up anyways with #29. I hope to do it similar to what you have done with this one abstract widget, so that I would only have to supply the request method. But in the meantime, I'd go with authenticationResult.

-> d57397c6eb29b0bcc21ac9630e3de510a39037b1

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:45

Commented on client/lib/widgets/common/image_provider_facade.dart line 4

-> a49c0c115d30f15433252f14758f00569ce4d569

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:45

Commented on .gitlab-ci.yml line 1

To be honest, I don't think it would be overly frustrating. We are talking about one or two minutes and if you have one failing build due to formatting errors, you (I hope) are checking your "style-fixes" by executing checkstyle locally or looking at the dart analysis tab in IntelliJ. I think your comment is a tad dramatic :)

The reasoning was that, in my opinion, check enforces stricter requirements than test. Code can be correct (test succeeds), but not "nice" (check fails). If test fails, I don't care about the result of check because the code is not deployable anyways. Only if test succeeds, it makes sense to have a look at check in order to verify deployability. This is all from the perspective of deploying build artifacts to some server via the CI.

In the end, if we really want to include continuous deployment (which would only be triggered for master commits or so), then we have to take a closer look anyways (how we want to do it, e.g. build only once in a separate job/stage, pass built files from one job to another in order not do build again in every job etc.). So, since you seem to prefer it the other way around, I have swapped it.

-> a810792c194c0dbad7d8c85844e81dbb7a1c2f2a

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 18:45

Commented on client/test/integration/integration_test.dart line 21

Apparently, I forgot to remove fruitless attempt at making the declaration of mocks nicer for test authors by introducing a typedef.

-> 342a2eb2e64a723add029fab4909079f18164d22

One thing that quite irks me is that in integration_test_utils, there is this registerSingleton (or registerDependency) method where I could do anything with the provided Injector object (besides only registering mocks, I could wreak some havoc by calling methods in ways I'm not supposed to). The same goes for AppArguments.overrideDependencies in the production code. My heart bleeds a little bit. Unfortunately, I didn't find a better way (without investing too much time) to solve it. Ideally, I'd supply two generic types (the "registered" type and the "supplier type" for the function that supplies an instance of the type or a subtype) and a function. But that's not possible - you cannot simply pass a Type and "do something" with it (e.g. var lala = T() or something like that), it's just a type reference. So I couldn't find a solution to this...potentially, it's a vulnerability because, as I said, consumers can to virtually anything with the Injector instance and mess up the whole dependency registration. But, as long as we don't have a nice solution for this (I really tried out some permutations of different approaches, but it all boiled down to the same, not-so-nice thing we are seeing right now), we have to trust ourselves not to be nasty.

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 19:16

resolved all threads

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 19:16

Commented on client/lib/widgets/authentication/login_card.dart line 46

Thanks!

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 19:16

Commented on .gitlab-ci.yml line 1

Maybe it's not that frustrating for you, but for me it is. I had the exact same problem when working at Kapsch, it was awful. Thanks for swapping it!

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 19:16

resolved all threads

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Nov 4, 2021, 19:16

approved this merge request

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 19:19

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Nov 4, 2021, 19:25

mentioned in commit c609169a1f5c5ce069437f1424b2882e8b110ac4