Samples: add automated test to sample 04.0-file-transfer #1565

pkirch · 2022-06-29T06:14:49Z

What this PR changes/adds

Automated integration tests that run in CI should be added for samples to prevent the samples breaking due to changes in the EDC. The scope of this story is to add automated integration tests for sample 04.0.

added test class FileTransferSampleTest.java
updated samples/04.0-file-transfer/README.md with missing sample output

Why it does that

Every step from sample readme is reproduced in the automated tests to reduce the risk from sample guidance diverging from actual code base.

Further notes

sample file samples/04.0-file-transfer/filetransfer.json was missing edctype set to dataspaceconnector:datarequest
changed FsConfigurationExtension.java to allow multiple EdcRuntimeExtension instances which load configuration from filesystem
- Adapted AzureDataFactoryTransferIntegrationTest.java to replicate previous behavior of FsConfigurationExtension.java. Without this change the check Test Code (Style, Tests) / Azure-Cloud-Integration-Test would fail.
no update to changelog as these changes apply to samples and other fixes seem minor

Linked Issue(s)

Closes #1269

Checklist

added appropriate tests?
performed checkstyle check locally?
added/updated copyright headers?
documented public classes/methods?
added/updated relevant documentation?
added relevant details to the changelog? (skip with label no-changelog)
formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

* add missing sample output to README.md * add missing property `edctype` to filetransfer.json * add missing toBuilder() method to DataRequest.java * add missing toBuilder() method to DataAddress.java * cache value for build root * test: add unit tests: verifyCopy, verifyDeepCopy, verifyToBuilder

codecov-commenter · 2022-06-29T06:25:45Z

Codecov Report

Merging #1565 (f9b66b9) into main (9679a32) will increase coverage by 0.30%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##             main    #1565      +/-   ##
==========================================
+ Coverage   67.63%   67.93%   +0.30%     
==========================================
  Files         739      739              
  Lines       16250    16259       +9     
  Branches     1062     1064       +2     
==========================================
+ Hits        10990    11046      +56     
+ Misses       4780     4733      -47     
  Partials      480      480

Impacted Files	Coverage Δ
...onnector/junit/extensions/EdcRuntimeExtension.java	`0.00% <ø> (ø)`
...taspaceconnector/junit/testfixtures/TestUtils.java	`35.00% <20.00%> (+0.51%)`	⬆️
...tor/configuration/fs/FsConfigurationExtension.java	`55.00% <33.33%> (ø)`
...nnector/spi/types/domain/transfer/DataRequest.java	`89.04% <92.85%> (+69.04%)`	⬆️
...taspaceconnector/spi/types/domain/DataAddress.java	`88.57% <100.00%> (+1.47%)`	⬆️

Continue to review full report at Codecov.

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

…mated-tests-sample-040

paullatzelsperger

I don't understand why the changes to DataRequest and DataAddress are necessary. The changes also modifies behaviour (e.g. properties).

pkirch · 2022-06-30T09:11:54Z

@paullatzelsperger wrote:

I don't understand why the changes to DataRequest and DataAddress are necessary. The changes also modifies behaviour (e.g. properties).

The changes in DataRequest and DataAddress follow the builder pattern as used in other classes in core-spi to enable immutable copies with changed values. There is a need in the test in readAndUpdateTransferJsonFile to read in DataRequest and DataAddress and write back a copy with changed values. Using this approach, we reuse the existing model and verify the given filetransfer.json file of the sample is still compatible with the current EDC. E.g. doing it like this surfaced already that filetransfer.json was missing "edctype": "dataspaceconnector:datarequest". Makes sense?

With changed behaviour you mean the use of address.properties.putAll(properties) instead properties.forEach(this::property)? That's a good catch! Indeed this "optimization" misses the null check now. I'll change that.

Apart from that I don't see changed behavior. Let me know if you see more.

paullatzelsperger · 2022-06-30T09:19:21Z

The changes in DataRequest and DataAddress follow the builder pattern as used in other classes in core-spi to enable

We won't add code to a class simply to make instantiating it in tests easier. If copying a data class is desired, then please add a utility method to that test.

Indeed this "optimization" misses the null check now

I'm not talking about nullchecks or optimizations. address.properties.putAll(properties) adds all properties (potentially replacing some), whereas before the properties were entirely replaced:

public Builder properties(Map<String, String> value) {
    request.properties = value;
    return this;
}

I would say that's a definite change in behaviour that is not desired at this time.

+ restore previous behavior for Builder.properties()

pkirch · 2022-06-30T13:48:15Z

We won't add code to a class simply to make instantiating it in tests easier. If copying a data class is desired, then please add a utility method to that test.

I see the point in scope creep here. I have moved the copy code into the test.

However, I have kept a fix and additional unit tests for DataRequest as there has been a shallow copy before and made a DataRequest instance mutable. If this should not be part of this PR, please let me know.

I'm not talking about nullchecks or optimizations. address.properties.putAll(properties) adds all properties (potentially replacing some), whereas before the properties were entirely replaced:
public Builder properties(Map<String, String> value) {
    request.properties = value;
    return this;
}
I would say that's a definite change in behaviour that is not desired at this time.

Indeed, this change in behavior was not intended. Reverted.

paullatzelsperger · 2022-06-30T14:12:26Z

One more time: we do not need/want the toBuilder(), copy() methods at this time. Either revert those changes or retract this PR.

If anything, DataRequest.copy() can be removed, as it is not used.

pkirch · 2022-06-30T16:55:34Z

One more time: we do not need/want the toBuilder(), copy() methods at this time.

Newly introduced toBuilder() and copy() methods have been already removed in the last push; no more to revert here.

Either revert those changes or retract this PR.

I have removed the fix from DataRequest and the regression tests from DataRequestTest. Those files are not part of this PR anymore.

ndr-brt · 2022-07-04T07:57:15Z

...c/test/java/org/eclipse/dataspaceconnector/extension/sample/test/FileTransferSampleTest.java

+     * @throws IOException Thrown if there was an error accessing the transfer request file defined in {@link FileTransferSampleTest#TRANSFER_FILE_PATH}.
+     */
+    void requestTransferFile() throws IOException {
+        File transferJsonFile = getFileFromRelativePath(TRANSFER_FILE_PATH);


Please use var

pkirch added the no-changelog label Jun 29, 2022

pkirch added 2 commits June 30, 2022 09:40

Merge branch 'eclipse-dataspaceconnector:main' into feature/1269-auto…

7fe1bf8

…mated-tests-sample-040

samples: fix checkstyle errors (#315)

f9b66b9

pkirch requested review from paullatzelsperger and jimmarino June 30, 2022 08:13

paullatzelsperger reviewed Jun 30, 2022

View reviewed changes

pkirch marked this pull request as ready for review June 30, 2022 08:41

pkirch requested review from ronjaquensel and juliapampus as code owners June 30, 2022 08:41

pkirch added 3 commits June 30, 2022 15:02

move DataAddress change code into test

d86981a

+ restore previous behavior for Builder.properties()

move DataRequest change code into test

5c53504

+ restore previous behavior for Builder.properties()

revert whitespace

419499b

pkirch added 3 commits June 30, 2022 18:32

reverted bug fix for shallow copy incl. regression tests

d29c5c8

reverted whitespace

91a5348

reverted whitespace

766879c

paullatzelsperger approved these changes Jul 1, 2022

View reviewed changes

pkirch mentioned this pull request Jul 1, 2022

Samples: 04.1-file-transfer-listener: add automated test in sample 04.1 agera-edc/DataSpaceConnector#317

Merged

7 tasks

ndr-brt approved these changes Jul 4, 2022

View reviewed changes

paullatzelsperger merged commit 8004b0f into eclipse-edc:main Jul 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Samples: add automated test to sample 04.0-file-transfer #1565

Samples: add automated test to sample 04.0-file-transfer #1565

pkirch commented Jun 29, 2022 •

edited

Loading

codecov-commenter commented Jun 29, 2022 •

edited

Loading

paullatzelsperger left a comment

pkirch commented Jun 30, 2022

paullatzelsperger commented Jun 30, 2022 •

edited

Loading

pkirch commented Jun 30, 2022

paullatzelsperger commented Jun 30, 2022

pkirch commented Jun 30, 2022

ndr-brt Jul 4, 2022

Samples: add automated test to sample 04.0-file-transfer #1565

Samples: add automated test to sample 04.0-file-transfer #1565

Conversation

pkirch commented Jun 29, 2022 • edited Loading

What this PR changes/adds

Why it does that

Further notes

Linked Issue(s)

Checklist

codecov-commenter commented Jun 29, 2022 • edited Loading

Codecov Report

paullatzelsperger left a comment

Choose a reason for hiding this comment

pkirch commented Jun 30, 2022

paullatzelsperger commented Jun 30, 2022 • edited Loading

pkirch commented Jun 30, 2022

paullatzelsperger commented Jun 30, 2022

pkirch commented Jun 30, 2022

ndr-brt Jul 4, 2022

Choose a reason for hiding this comment

pkirch commented Jun 29, 2022 •

edited

Loading

codecov-commenter commented Jun 29, 2022 •

edited

Loading

paullatzelsperger commented Jun 30, 2022 •

edited

Loading