test: Expand backup integration tests #1699
Conversation
AndrewSisley
added
area/db-system
AndrewSisley
force-pushed
the
1696-import-self-ref-tests
branch
from
7cf43f3
to
d237572
Compare
Codecov ReportPatch coverage has no change and project coverage change:
@@ Coverage Diff @@
## develop #1699 +/- ##
===========================================
- Coverage 75.42% 75.38% -0.04%
===========================================
Files 208 208
Lines 21694 21694
===========================================
- Hits 16362 16353 -9
- Misses 4195 4201 +6
- Partials 1137 1140 +3
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.
|
| ImportContent: `{ | ||
| "User":[ | ||
| { | ||
| "_key":"bae-790e7e49-f2e3-5ad6-83d9-5dfb6d8ba81d", |
There was a problem hiding this comment.
suggestion: Since _newKey will be used to check if it's a self reference, I suggest we change _key for _newKey or have both.
There was a problem hiding this comment.
_newKeywill be used to check if it's a self reference
It would? I always saw it the other way round, as any exported relation id fields would contain the old key, not the new one.
Happy to have both.
- [ ] add _newKey
There was a problem hiding this comment.
The relation ID will be updated on export.
There was a problem hiding this comment.
Ah, okay nice :)
But wouldn't it make more sense for the export to contain _oldKey and _key then (instead of _newKey)? As the docs in the export reference the new key, not the old one?
There was a problem hiding this comment.
Converted to the import-export flow suggested in another comment
| executeTestCase(t, test) | ||
| } | ||
|
|
||
| func TestBackupImport_WithMultipleNoKeysAndInvalidField_NoError(t *testing.T) { |
There was a problem hiding this comment.
suggestion: Change function name to end with _ReturnError as that is what is expected of the function under test.
There was a problem hiding this comment.
My bad, will change, thanks
- Fix test name
|
|
||
| // This test documents undesirable behaviour, as the documents are not linked. | ||
| // https://github.com/sourcenetwork/defradb/issues/1697 | ||
| func TestBackupSelfRefImport_SplitPrimaryRelationWithSecondCollection_NoError(t *testing.T) { |
There was a problem hiding this comment.
suggestion: This test would work if the import content was taken from the generated export. I suggest either it documents an invalid import content and we remove the linked issue above the function name or we change the test to use a valid import content.
There was a problem hiding this comment.
That's fair, and it is more descriptive, even if a little longer/more-complex. Will change.
- import an export (and in the other similar test)
AndrewSisley
force-pushed
the
1696-import-self-ref-tests
branch
2 times, most recently
from
2fca8f8
to
676d8dd
Compare
Previously only half of the relation was defined, this was invalid.
AndrewSisley
force-pushed
the
1696-import-self-ref-tests
branch
from
676d8dd
to
842db2b
Compare
| } | ||
|
|
||
| // This test documents undesirable behaviour, as the documents are not linked. | ||
| // https://github.com/sourcenetwork/defradb/issues/1697 |
There was a problem hiding this comment.
note: This test isn't linked to an import error. It's an export error. It's having trouble setting the right expected book ID because they both have a primary field related to each other.
There was a problem hiding this comment.
Ah nice spot - would be good to fix that too if you have time, I'm not sure your fix-PR solves for this kind of schema anyway and it is hard to test fully independently of the import
There was a problem hiding this comment.
We can't test it on import. It has to be tested on export. The json object representing the exported data is clearly wrong so we could correct it manually and assert an error instead, Showing a wrong output after import is fine. Just a little less clear as to what the problem is.
There was a problem hiding this comment.
Ah true - good shout. Tweaking now, will link to a new ticket.
- Different bug
## Relevant issue(s) Resolves sourcenetwork#1696 ## Description Expands the backup integration tests, and fixes an invalid schema declaration. Documents sourcenetwork#1697 I went over the CLI tests too, but found no gaps.
Relevant issue(s)
Resolves #1696
Description
Expands the backup integration tests, and fixes an invalid schema declaration.
Documents #1697
I went over the CLI tests too, but found no gaps.