test(i): Test UpdateDoc tests with gql and Collection.Save

[AndrewSisley]

Relevant issue(s)

Resolves #1816

Description

Adds a new make command and CI action to test any test with a defined UpdateDoc test action using an equivalent GQL request (using id param to target). This enforces consistency across both (future=all) mutation pathways, without any notable effort from us (e.g. compared to explicitly writing tests for both/all, and changing them all if/whenever syntax/behaviour changes).

CreateDoc and DeleteDoc should receive support for this later if we are happy with the approach. collection.UpdateWithKey and collection.UpdateWithKeys could also be added later if we wish (my preference).

The new CI action runs without race, shuffle etc. and only using the Badger memory store. I don't think it is worth complicating it at the moment.

It does not change the code-cov action, so any extra lines tested via the new action will not be reflected there. We can have a look at doing that later (I'd also like the same for the change detector).

Post Review tasks

• If/when reviewers are happy, create tickets for CreateDoc and DeleteDoc, and Collection.UpdateWithKey and Collection.UpdateWithKeys.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (9b41fb5) 75.78% compared to head (2246a71) 75.77%.

@@             Coverage Diff             @@
##           develop    #1818      +/-   ##
===========================================
- Coverage    75.78%   75.77%   -0.01%     
===========================================
  Files          209      209              
  Lines        22268    22268              
===========================================
- Hits         16874    16872       -2     
- Misses        4229     4230       +1     
- Partials      1165     1166       +1     

Flag	Coverage Δ
all-tests	75.77% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b41fb5...2246a71. Read the comment docs.

[fredcarle]

I like this especially since it's on its own ci action and can run in parallel to the other ones.

I would encourage you to wait for other team members review to get their feedback in case they have a different opinion.

[AndrewSisley]

I like this especially since it's on its own ci action and can run in parallel to the other ones.

Cheers :)

I would encourage you to wait for other team members review to get their feedback in case they have a different opinion.

Yeah, was planning on leaving this one for a few days to see if it gets any extra attention, is not just a quick bug fix and will impact all of us.

[shahzadlone]

I just have some thoughts, will think out loud here I am unsure myself if I like this approach or not:

• We have discussed previously in our standup where we were trying to shorten the CI developer wait time to:

  (1) Split the test runs such that every workflow runs the test against one datastore (as opposed to the current model where we run against multiple in one workflow)

  (2) Find a way to run in a parallel fashion make test, make test:cli, make test:lens as they are independent, under one workflow (one job possibly), rather than current serialized approach.

Note: The make commands in (2) are all ran with race flags on.

• How does (1) and (2) play with this Pull Request? I would like to avoid getting to a place of "ci-workflow hell" where we are further dividing every split test workflow in (1) that we talked about to each have a corresponding workflow to test Collection.Save.

• I would much prefer the Collection.Save test be part of the (2) solution to be ran within the same per-datastore workflow rather than it's own workflow. As tomorrow we might want to then introduce another workflow to test another component.

• As for codecov, it can generate a codecov report in a 1-1 manner according to (2) running the same new concurrent test commands and upload their corresponding reports (batched into one report per each datastore workflow).

• Similar to codecov example above and (2) change detector can also run in a parallel fashion, i) the tests as the previously ran with change detector and this new flag within the change detector (once for each datastore workflow).

[AndrewSisley]

The make commands in (2) are all ran with race flags on.

Not in the new jobs/make-commands they are not.

(1) Split the test runs such that every workflow runs the test against one datastore

I am very much against this, unless I am misunderstanding how you picture this. Each test should be fully independent and should not share state (including the datastore). I however do know what you are hoping to gain by this.

However, assuming that we do want (1), I dont see this as particularly relevant. There is nothing in this PR that should prohibit such a setup.

(2) Find a way to run in a parallel fashion make test, make test:cli, make test:lens as they are independent, under one workflow (one job possibly), rather than current serialized approach.

Yes, it is not terribly difficult either, but within this PR is not the place for such work. Although, I do think there are very good organisational reasons to want to split those into different workflows in the future (think CLI team, vs core team etc).

"ci-workflow hell" where we are further dividing every split test workflow

We can always 'refactor' the workflows, essentially grouping multiple under a single workflow. There are many ways in which we can parallelise stuff without creating new workflows, multiple workflows is the easiest for now, and I dont see a good reason to not use it until we gain a fair few more (although with Keenan's work that time might come rather soon).

I would much prefer the Collection.Save test be part of the (2) solution to be ran within the same per-datastore workflow rather than it's own workflow.

Discussed above. I consider this to be out of scope.

As for codecov...

There is a bunch of stuff we can do. All of which I consider to be out of scope, as it is not a new problem.

change detector can also run in a parallel fashion

There are two things that are stopping the change detector tests from running in parallel; machine resources, and the git clone stuff. Both are irrelevant to this PR, and the github workflow setup.

[shahzadlone]

The make commands in (2) are all ran with race flags on.

Not in the new jobs/make-commands they are not.

Yes I am aware of the new ones introduced are not, but I thought the plan for (2) when we discussed on the call was to hold all the existing testing guarantees (i.e. with race) and to split and parallelize accordingly.

follow-up-question: What was the reason to skip race and shuffle for this new job?

(1) Split the test runs such that every workflow runs the test against one datastore

I am very much against this, unless I am misunderstanding how you picture this. Each test should be fully independent and should not share state (including the datastore). I however do know what you are hoping to gain by this.

Not sure if I follow, or if we are talking about the same thing. What I thought we were brainstorming on the call was that currently, the ci runs the tests on both DEFRA_BADGER_MEMORY and DEFRA_BADGER_FILE. Thought there was consensus to try out having 2 workflows such that tests are ran in one workflow #1 where only DEFRA_BADGER_MEMORY=true and workflow #2 where only DEFRA_BADGER_FILE=true and in total have 2 workflows with shorter time, rather than 1 workflow with a longer time. This is what (1) is.

However, assuming that we do want (1), I dont see this as particularly relevant. There is nothing in this PR that should prohibit such a setup.

Agreed, it doesn't prohibit it. Just trying to have a discussion in regards to what the final or longterm will now look like with this and (1) in the picture. Would it be that we now need to have this new workflow with both a DEFRA_BADGER_FILE workflow and a DEFRA_BADGER_MEMORY or keep it as is?

(2) Find a way to run in a parallel fashion make test, make test:cli, make test:lens as they are independent, under one workflow (one job possibly), rather than current serialized approach.

Yes, it is not terribly difficult either, but within this PR is not the place for such work. Although, I do think there are very good organisational reasons to want to split those into different workflows in the future (think CLI team, vs core team etc).

"ci-workflow hell" where we are further dividing every split test workflow

We can always 'refactor' the workflows, essentially grouping multiple under a single workflow. There are many ways in which we can parallelise stuff without creating new workflows, multiple workflows is the easiest for now, and I dont see a good reason to not use it until we gain a fair few more (although with Keenan's work that time might come rather soon).

I would like to avoid splitting into too many workflows, because then combining with (1) we will end up in the ci workflow hell I mentioned before.

For example just for running the tests we will have like 12 workflows:

1. Run normal tests - In DefraDB Memory
2. Run Cli tests - In DefraDB Memory
3. Run lens tests - In DefraDB Memory
4. Run UpdateDoc with Collection.Save tests - In DefraDB Memory
5. Run normal tests - In Badger Memory
6. Run Cli tests - In Badger Memory
7. Run lens tests - In Badger Memory
8. Run UpdateDoc with Collection.Save tests - In Badger Memory
9. Run normal tests - In Badger File
10. Run Cli tests - In Badger File
11. Run lens tests - In Badger File
12. Run UpdateDoc with Collection.Save tests - In Badger File

And similar number for code cov, then change the detector workflows? That's like 36ish workflows which will all have bootup times and build times and etc.

Instead, If we only split by type of store we are testing then we can get away with 3 workflows and all of them doing the parallelization within them.

I would much prefer the Collection.Save test be part of the (2) solution to be ran within the same per-datastore workflow rather than it's own workflow.

Discussed above. I consider this to be out of scope.

Sure it can be out of scope of this PR, but the thought is not asking to change the scope or add new things, it's trying to discuss where we want to get to eventually as the proposed changes are totally opposite to the suggested preference I have mentioned. If the goal of this PR is to quickly throw together a workflow that catches the bugs that were leaked previously and come back to properly rethinking the long-term goals of how this integrates into the CI flow, that is fine and the PR is good to merge as is in that case, but we still have to be mindful of coming back and documenting that this is something we want to change.

As for codecov...

There is a bunch of stuff we can do. All of which I consider to be out of scope, as it is not a new problem.

Similar to above point, can be out of scope, however important to know the direction we want to take specially with other discussions we have had to reduce CI wait time, this is a new workflow that also takes 8mins or so.

change detector can also run in a parallel fashion

There are two things that are stopping the change detector tests from running in parallel; machine resources, and the git clone stuff. Both are irrelevant to this PR, and the github workflow setup.

Machine resources we I think have to eventually accommodate inorder to save time and don't have to worry much from implementation prespective, the github cloning part will be interesting. However my question was more from the prespective of if we want to run change detector two times now? once with Collection.Save and once the way we were previously? and parallelization between those two runs (not parallelization between one change detection run).

[shahzadlone]

todo: All workflows should end with the Workflow ending.

Suggested change

	name: Test GQL Mutations
	name: Test GQL Mutations Workflow

[AndrewSisley]

Ugh, I thought I matched all the naming conventions I could spot, will change.

• Rename workflow (see other comment too)

[shahzadlone]

nitpick: the other workflow is called "Run Tests Workflow" So would have a preference of calling this workflow "Run GQL Mutation Tests Workflow"

[AndrewSisley]

when we discussed on the call was to hold all the existing testing guarantees

That was for existing 'core' tests. This is something sort of new, and wasn't part of that conversation.

follow-up-question: What was the reason to skip race and shuffle for this new job?

This was noted in the description, I just didn't think it was worth it atm.

ci runs the tests on both DEFRA_BADGER_MEMORY and DEFRA_BADGER_FILE

Got it - you mean datastore type - I thought you were talking about all the tests sharing a datastore instance and I thought I had missed a convo somewhere 😁

For example just for running the tests we will have like 12 workflows

We'd only have one flow for this job, no need to run it per datastore type. I don't see us really gaining anything by doing that atm. We can do so later if we want, but such a thing is well out of scope (also noted in the description).

And similar number for code cov, then change the detector workflows

Again, I question the relevancy. We have one new workflow in this PR, in addition to 12 existing (of which only 3 are tests).

t's trying to discuss where we want to get to eventually... the proposed changes are totally opposite

Long term I have no idea. Any serious discussion around that is premature. In this PR we have a test gap, and this closes it. There is nothing in here that affects any future refactor of the test jobs etc. It is not totally opposite at all.

this is a new workflow that also takes 8mins or so

And it will shrink using whatever means we use to shrink the others. There is nothing special about this workflow at all.

However my question was more from the prespective of if we want to run change detector two times now

I don't know, and, particularly within the context of this PR, I do not care. It is not relevant.

[AndrewSisley]

@shahzadlone

I think you might be overthinking/complicating this. It is extremely cheap to change the workflow if/when we want to. If in the future we wish to run this for multiple datastore types, or within the change detector too, or with race or whatever, the github action stuff can be changed to accommodate that sensibly.

Atm, we have 3 existing test flows, and a new one. 4 is not a problem IMO.

[shahzadlone]

That was for existing 'core' tests. This is something sort of new, and wasn't part of that conversation.

Indeed is why wanted to get a sense of how we plan to integrate these in future.

This was noted in the description, I just didn't think it was worth it atm.

Lemme rephrase the question then haha, why is it not worth it atm, is it because the time of CI will be increased, the race in this code path doesn't provide much value, or another one. Just curious.

Got it - you mean datastore type - I thought you were talking about all the tests sharing a datastore instance and I thought I had missed a convo somewhere 😁

Haha yea, no lol I was talking about the datastore type not instance.

We'd only have one flow for this job, no need to run it per datastore type. I don't see us really gaining anything by doing that atm. We can do so later if we want, but such a thing is well out of scope (also noted in the description).

All I saw was the "what" aspect in the description: The new CI action runs without race, shuffle etc. and only using the Badger memory store. was just trying to understand more the "why" so in future we can be mindful what this test is for, and that we don't need to worry about running this on unnecessary datastore types than needed. Thanks for explaining that one flow is sufficient for this test, is the answer I was looking for.

I think you might be overthinking/complicating this. It is extremely cheap to change the workflow if/when we want to. If in the future we wish to run this for multiple datastore types, or within the change detector too, or with race or whatever, the github action stuff can be changed to accommodate that sensibly.

It's only cheap to change the workflow as long as we are all on the same page and understand the limitations/purposes of the current state. If there is a difference of understanding in what a workflow or check is for, then we can end up building a bigger mess in the name of "accommodating a previous check that people might not be aware of" during a refactor. Therefore wanted to clarify some points and think out loud.

Atm, we have 3 existing test flows, and a new one. 4 is not a problem IMO.

Atm it is not, agreed. I also see a distinction between running test, test coverage and detecting change flows. Currently we have one running test workflow, now we have 2. So to clarify, we'd in the near future roughly want 3 workflows to run tests (two for datastore states, and +1 for Collection.Save), in that case I agree it's not bad even in "near future".

Also you already have an approval, I only had a todo and nitpick, no changes requested these are all non-blocking discussions I wanted to have, thanks for answering these.

[AndrewSisley]

Why is it not worth it atm, is it because the time of CI will be increased, the race in this code path doesn't provide much value, or another one?

Race is of lower value IMO. At the moment we have very few tests, this PR adds coverage comparable with collection.Save - this is a relatively very large gain IMO. The race flag may be nice in the future, but the value gain is not comparable atm IMO and can be done later if/when we want.

Thanks for explaining that one flow is sufficient for this test, is the answer I was looking for

I think it is for now, but that is largely because 1 is better than 0 :) I think testing against others may be good in the future, but should be looked at in more detail when we want to spend time on such things.

It's only cheap to change the workflow as long as we are all on the same page and understand the limitations/purposes of the current state.

This is kind of a nice point, and at the same time I kind of disagree 😁 Our understanding will not be the same tomorrow as it is today. If/when we come to change this workflow, we will very likely need to largely rely on the source of truth - the code/config. We will also likely have far better questions to ask of it when that time comes.

At PR time, I think it is best to focus on whether the current state is understandable (otherwise you cant review it, and it is the best indication as to whether you can understand it later when it really matters), whether or not it is better than what we currently have, and whether or not it is going to be really expensive to change later. I (largely) see guessing at the future a waste of energy, and a frequent cause of pointless never-used code complications (I did this recently).

By focusing on whether it is easy (and safe) or not to change, it doesn't really matter what we want in the future. It should be able to accommodate it with minimal hassle.