test(i): Extend mutation tests with col.Update and Create #1838
Conversation
AndrewSisley
added
area/collections
AndrewSisley
changed the title
Codecov ReportPatch coverage has no change and project coverage change:
@@ Coverage Diff @@
## develop #1838 +/- ##
===========================================
- Coverage 75.81% 75.78% -0.03%
===========================================
Files 209 209
Lines 22261 22261
===========================================
- Hits 16875 16869 -6
- Misses 4223 4227 +4
- Partials 1163 1165 +2
Flags with carried forward coverage won't be shown. Click here to find out more. see 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Left some minor todos. Giving an LGTM assuming you will resolve all those.
I know you mentioned that you disagree with being on the same page and understanding the limitations/purposes of the current state, as we might not have the same understanding tomorrow (#1818 (comment)). While I understand and partially-agree with your point, I think we can do both, discuss the limitations or merely leave indexable thoughts at the point of change, and also decide when the time comes for the refactor or improvement.
So will leave these non-blocking thoughts here:
-
I do still wonder about getting closer and closer to the "ci workflow hell" I mentioned in the comment here test(i): Test UpdateDoc tests with gql and Collection.Save #1818 (comment)
-
Looks like some expectations I thought in this comment(test(i): Test UpdateDoc tests with gql and Collection.Save #1818 (comment)) won't be true and so want to expand on the future that I think we will be getting to for the middle-term at least. In the near future roughly will have 5 workflows that will run tests in the CI (two for datastore states, and +1 for Col.Save, + 1 Col.Update, +1 Col.Delete).
| @@ -0,0 +1,61 @@ | |||
| # Copyright 2022 Democratized Data Foundation | |||
There was a problem hiding this comment.
todo:
| # Copyright 2022 Democratized Data Foundation | |
| # Copyright 2023 Democratized Data Foundation |
There was a problem hiding this comment.
- Fix date in cw header
| # by the Apache License, Version 2.0, included in the file | ||
| # licenses/APL.txt. | ||
|
|
||
| name: Run Collection.[Named] Mutations Tests Workflow |
There was a problem hiding this comment.
todo: I don't like the name to be technical, it should be descriptive in english.
| name: Run Collection.[Named] Mutations Tests Workflow | |
| name: Run Collection Named Mutations Tests Workflow |
There was a problem hiding this comment.
I'll change it, but I have to ask why, given the presumed target audience.
- Rename wf name
|
|
||
| jobs: | ||
| test-gql-mutations: | ||
| name: Test Collection.[Named] Mutations job |
There was a problem hiding this comment.
todo: Same as above suggestion.
| name: Test Collection.[Named] Mutations job | |
| name: Test Collection Named Mutations job |
There was a problem hiding this comment.
- Rename wf job name
| - develop | ||
|
|
||
| jobs: | ||
| test-gql-mutations: |
There was a problem hiding this comment.
todo: Job name should be also consistent.
| test-gql-mutations: | |
| test-collection-named-mutations: |
There was a problem hiding this comment.
My bad, didn't spot this - thanks.
- fix name
| ## Uncomment to enable ability to SSH into the runner. | ||
| #- name: Setup upterm ssh session for debugging | ||
| # uses: lhotari/action-upterm@v1 | ||
| # with: | ||
| # limit-access-to-actor: true | ||
| # limit-access-to-users: shahzadlone |
There was a problem hiding this comment.
todo: Removal, this is enough to have in one file, no need to have in everyfile.
There was a problem hiding this comment.
How do you find the file in which it is actually defined? But will remove.
- Remove comment thing
There was a problem hiding this comment.
just removing here should be fine, I think unless introduced also in last PR that added a new workflow it is only in one file (the you based your workflow from originally).
Given that you seem happy enough to tolerate the current setup in the short term, and that we have already talked about this a little bit, I only wish to highlight the relative costs of some related actions:
I do not think further discussion on this topic is sensible, unless there is a problem with the immediate, real, state. |
AndrewSisley
force-pushed
the
1832-mut-tests-upcreate
branch
from
e28e9f8
to
ab9f03e
Compare
AndrewSisley
force-pushed
the
1832-mut-tests-upcreate
branch
from
44aef2e
to
a5989ed
Compare
AndrewSisley
force-pushed
the
1832-mut-tests-upcreate
branch
from
d1ec7dc
to
e54477e
Compare
I strongly disagree with micro-analyzing every small discussion(question) someone might want to do(ask) and multiplying everyone's time in a meeting when it was brought up by someone else who wanted a clarification on the discussion and grouping these as "unsensible discussions or time wasting discussions". I would be very careful walking down this path of discouraging discussions / communications. It's important to keep in mind that as an author of a PR you have a lot of context and information that others don't, what might seem as silly or time wasting or unsensible questions to you might be questions to others that would help them get one step closer to understanding your vision. I think this is one of the values we should have engrained into source engineers and ecosystem users to not feel ashamed or hesitant in asking questions. So, I will continue to ask these non-sensible questions. |
The reason for writing that was not to micro-analyize time, but to highlight that this thing (imagined future workflow stuff) does not matter. The importance of different parts of our codebase varies significantly, and I consider it very sensible to not worry about large chunks of stuff. This is true at review time, and when authoring code. Attention is a finite resource, and attention given to unimportant elements is attention taken from those that need it. Discussion is good, and healthy, but it needs to be done within the correct time and space. If a PR is dominated by a conversation about an unimportant aspect, then it increases the risk of very real issues being missed elsewhere within it. PRs are a serious, time-sensitive space, and conversations should focus on stuff that really matters, within the context of that PR. I stand by the recent times where I have tried to close out conversations within PRs because of this. This does not mean however that the conversations cannot continue elsewhere, in a less-important space/time. Similar to how we don't tag every discord message with |
…ork#1838) ## Relevant issue(s) Resolves sourcenetwork#1832 ## Description Extends mutation tests with collection.Update and Create calls for the corresponding actions. Adds another github workflow to run the tests.
Relevant issue(s)
Resolves #1832
Description
Extends mutation tests with collection.Update and Create calls for the corresponding actions. Adds another github workflow to run the tests.
I didn't really plan on adding this so soon, but I really want it for #935, which I want for #1331 - as well as just being nice to test these :)
Is based off #1837 - please don't review the first commit here.