fix: add missing json fields for sample streaming http-to-http #335

klumbe · 2024-11-06T15:04:45Z

What this PR changes/adds

Fixes issues #244 and #334 for sample transfer/streaming/streaming-01-http-to-http

Missing assigner field in contract request
Missing transferType in transfer request
Did not use JSON files in testcase

Why it does that

Sample can not be executed completely.

Further notes

Linked Issue(s)

Closes #244
Closes #334

github-actions

'We are always happy to welcome new contributors ❤️ To make things easier for everyone, please - make sure to follow our contribution guidelines, - check if you have already signed the ECA, and - relate this pull request to an existing issue or discussion.'

ndr-brt

while are you working on this field, could you change the Streaming01httpToHttpTest to use these files instead of building the requests from scratch?
You can get some inspiration from Transfer02consumerPullTest (that already does this).

plus, please update the PR name according to the convention? (info here)

klumbe · 2024-11-07T13:09:11Z

@ndr-brt:
Since I can't find any doc about how to run the test-cases, I can't fix it that easily.
The official doc returns 404 for almost every link in there (i.e. https://eclipse-edc.github.io/testing.md found in https://eclipse-edc.github.io/documentation/for-contributors/#52-testing-best-practices).

I flew over the convention slightly, does the new name match?

ndr-brt · 2024-11-07T13:13:18Z

@ndr-brt: Since I can't find any doc about how to run the test-cases, I can't fix it that easily. The official doc returns 404 for almost every link in there (i.e. https://eclipse-edc.github.io/testing.md found in https://eclipse-edc.github.io/documentation/for-contributors/#52-testing-best-practices).

I flew over the convention slightly, does the new name match?

link is broken, but you can find it under contributors manual/writing tests

klumbe · 2024-11-07T14:17:51Z

link is broken, but you can find it under contributors manual/writing tests

Almost all links are broken, unfortunately ;-)

For me, it still does not work for half of the test cases, since Testcontainers does not recognize the docker-engine.
Using the official Docker engine (no Desktop) on Ubuntu 24.04. User is in correct group and can contact the daemon.
Path /var/run/docker.sock exists and has rw access for the group the user is in.
Tried it with setting the DOCKER_HOST variable as well. No change.

ndr-brt · 2024-11-07T14:42:24Z

link is broken, but you can find it under contributors manual/writing tests

Almost all links are broken, unfortunately ;-)

contributions are the key ;)

For me, it still does not work for half of the test cases, since Testcontainers does not recognize the docker-engine. Using the official Docker engine (no Desktop) on Ubuntu 24.04. User is in correct group and can contact the daemon. Path /var/run/docker.sock exists and has rw access for the group the user is in. Tried it with setting the DOCKER_HOST variable as well. No change.

I'm also using docker engine on ubuntu without issues, it worked out-of-the-box, there's no need to do changes, as long as docker is executable from the command line by a non root user.
what's the error?

klumbe · 2024-11-07T15:44:58Z

link is broken, but you can find it under contributors manual/writing tests

Almost all links are broken, unfortunately ;-)

contributions are the key ;)

For me, it still does not work for half of the test cases, since Testcontainers does not recognize the docker-engine. Using the official Docker engine (no Desktop) on Ubuntu 24.04. User is in correct group and can contact the daemon. Path /var/run/docker.sock exists and has rw access for the group the user is in. Tried it with setting the DOCKER_HOST variable as well. No change.

I'm also using docker engine on ubuntu without issues, it worked out-of-the-box, there's no need to do changes, as long as docker is executable from the command line by a non root user. what's the error?

Strange.
Basically, the long exception-message starts with:

Advanced01openTelemetryTest > runSampleSteps() FAILED
    java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration
        at org.testcontainers.dockerclient.DockerClientProviderStrategy.lambda$getFirstValidStrategy$7(DockerClientProviderStrategy.java:274)

klumbe · 2024-11-07T16:23:07Z

It seems to be one of those funny things where installing OS updates and rebooting fixes it.
Can't explain why, since OS was pretty up-to-date and settings seemed right, but it worked.

klumbe · 2024-11-07T21:31:51Z

@ndr-brt: Took me a long time, but I fixed it.
Not sure why ECA does not evaluate anymore, or why it evaluated before, since I am using my anonymized Github email.
I filled out the form though.

ndr-brt · 2024-11-08T07:40:44Z

@ndr-brt: Took me a long time, but I fixed it. Not sure why ECA does not evaluate anymore, or why it evaluated before, since I am using my anonymized Github email. I filled out the form though.

I think it is becuse your first commit, that has no email reported: 2f00b9d

maybe you could squash the three commits into one that has the correct info

checkstyle warnings need to be fixed as well

klumbe · 2024-11-08T09:48:18Z

@ndr-brt:
You mean the last one? Indeed this one was missing the author completely.
Shame on me.
Fixed that by a rebase / amend, should be better now.

Haven't seen the checkstyle errors during testing, fixed those as well.
Warnings were there before, I am not about to fix those as well.
So it should be good if all Github checks pass.

Btw: Is there a way to only run a certain test? It was quite annoying, since though I tried some things from the Gradle docs,
I could only get all tests running by firing:

.\gradlew test -DrunAllTests="true"

ndr-brt · 2024-11-08T10:42:35Z

@ndr-brt: You mean the last one? Indeed this one was missing the author completely. Shame on me. Fixed that by a rebase / amend, should be better now.

Haven't seen the checkstyle errors during testing, fixed those as well. Warnings were there before, I am not about to fix those as well. So it should be good if all Github checks pass.

Btw: Is there a way to only run a certain test? It was quite annoying, since though I tried some things from the Gradle docs, I could only get all tests running by firing:
.\gradlew test -DrunAllTests="true"

if you are using Intellij you just need to add the -DrunAllTests="true" in the run configuration.
you could also create a configuration template and make it enabled for all the gradle commands (very handy for EDC projects)

klumbe · 2024-11-08T11:33:53Z

@ndr-brt: You mean the last one? Indeed this one was missing the author completely. Shame on me. Fixed that by a rebase / amend, should be better now.
Haven't seen the checkstyle errors during testing, fixed those as well. Warnings were there before, I am not about to fix those as well. So it should be good if all Github checks pass.
Btw: Is there a way to only run a certain test? It was quite annoying, since though I tried some things from the Gradle docs, I could only get all tests running by firing:
.\gradlew test -DrunAllTests="true"
if you are using Intellij you just need to add the -DrunAllTests="true" in the run configuration. you could also create a configuration template and make it enabled for all the gradle commands (very handy for EDC projects)

But this would still run all tests, doesn't it?
Since it was pretty basic testing so far, the setup is quite simple (no sophisticated IDE).
May have to change that once I start real development.

Checks look good now, right?

ndr-brt · 2024-11-08T12:33:36Z

if you are using Intellij you just need to add the -DrunAllTests="true" in the run configuration. you could also create a configuration template and make it enabled for all the gradle commands (very handy for EDC projects)

But this would still run all tests, doesn't it? Since it was pretty basic testing so far, the setup is quite simple (no sophisticated IDE). May have to change that once I start real development.

no, this will say to junit: "run the test you want without considering the tag attached to it.

ndr-brt

a little thing to fix then we'll be ready to merge

ndr-brt · 2024-11-08T12:33:58Z

...ests/src/test/java/org/eclipse/edc/samples/transfer/streaming/Streaming01httpToHttpTest.java

+        var catalogDatasetId = CONSUMER.fetchDatasetFromCatalog(getFileContentFromRelativePath(SAMPLE_FOLDER + "/get-dataset.json"));
+        var negotiateContractBody = getFileContentFromRelativePath(SAMPLE_FOLDER + "/negotiate-contract.json")
+                                        .replace("{{offerId}}", catalogDatasetId);
+        //assertThat(catalogDatasetId).isEqualTo("test", catalogDatasetId);


commented out line should be removed

Oh, good catch.
Had been too long yesterday, obviously.
That was my way to find out how to fix the issue that jsonPath rejects colons if not escaped.
A bit tricky when it is the first time to use it at all ;-)

I have amended it to the last commit to not have too many.

klumbe · 2024-11-08T14:23:11Z

if you are using Intellij you just need to add the -DrunAllTests="true" in the run configuration. you could also create a configuration template and make it enabled for all the gradle commands (very handy for EDC projects)

But this would still run all tests, doesn't it? Since it was pretty basic testing so far, the setup is quite simple (no sophisticated IDE). May have to change that once I start real development.

no, this will say to junit: "run the test you want without considering the tag attached to it.

Ok, might work with IDEA. You don't know a way to run a specific test through CLI?

ndr-brt · 2024-11-11T13:59:10Z

Ok, might work with IDEA. You don't know a way to run a specific test through CLI?

sure, for this one just execute:

./gradlew :system-tests:test --tests "org.eclipse.edc.samples.transfer.streaming.Streaming01httpToHttpTest.streamData" -DrunAllTests=true

klumbe · 2024-11-11T15:06:40Z

Ok, might work with IDEA. You don't know a way to run a specific test through CLI?

sure, for this one just execute:
./gradlew :system-tests:test --tests "org.eclipse.edc.samples.transfer.streaming.Streaming01httpToHttpTest.streamData" -DrunAllTests=true

Haven't tried it yet, but sounds as if it could work. Thank you.
I had tried it basically without the -DrunAllTests=true, since I thought it would do the opposite of what I wanted to achieve.
I read about the fact that otherwise system-tests would not be executed at all, but I thought it would be sort of a side-effect of running all existing test-cases.
Might be a case where we first enlarge the scope to then filter out.
Not too familiar with Gradle unfortunately.

ndr-brt · 2024-11-11T15:12:41Z

Haven't tried it yet, but sounds as if it could work. Thank you. I had tried it basically without the -DrunAllTests=true, since I thought it would do the opposite of what I wanted to achieve. I read about the fact that otherwise system-tests would not be executed at all, but I thought it would be sort of a side-effect of running all existing test-cases. Might be a case where we first enlarge the scope to then filter out. Not too familiar with Gradle unfortunately.

the -DrunAllTests=true basically says "run the selected tests despite the junit tag", given the fact that this test has the "EndToEnd" label, that property is a sort of passepartout to not care about the labels and just run the tests.

For this test, this command does the same exact thing as the one before

./gradlew :system-tests:test --tests "org.eclipse.edc.samples.transfer.streaming.Streaming01httpToHttpTest.streamData" -DincludeTags="EndToEndTest"

ndr-brt

anyway, thanks for the contribution, I'm gonna merge this 🚀

klumbe · 2024-11-11T16:25:35Z

Haven't tried it yet, but sounds as if it could work. Thank you. I had tried it basically without the -DrunAllTests=true, since I thought it would do the opposite of what I wanted to achieve. I read about the fact that otherwise system-tests would not be executed at all, but I thought it would be sort of a side-effect of running all existing test-cases. Might be a case where we first enlarge the scope to then filter out. Not too familiar with Gradle unfortunately.

the -DrunAllTests=true basically says "run the selected tests despite the junit tag", given the fact that this test has the "EndToEnd" label, that property is a sort of passepartout to not care about the labels and just run the tests.

For this test, this command does the same exact thing as the one before
./gradlew :system-tests:test --tests "org.eclipse.edc.samples.transfer.streaming.Streaming01httpToHttpTest.streamData" -DincludeTags="EndToEndTest"

Thank you for the explanation (and preventing me from dying uneducated ;-)).

klumbe · 2024-11-11T16:26:10Z

anyway, thanks for the contribution, I'm gonna merge this 🚀

Perfect, thanks. You are welcome. Hope it helps somebody.

klumbe added 2 commits November 6, 2024 15:56

fix: add missing assigner to contract request eclipse-edc#244

e70a172

fix: add missing transferType to transfer request eclipse-edc#334

4b0d194

github-actions bot reviewed Nov 6, 2024

View reviewed changes

ndr-brt self-requested a review November 7, 2024 07:58

ndr-brt reviewed Nov 7, 2024

View reviewed changes

ndr-brt added the bug Something isn't working label Nov 7, 2024

klumbe changed the title ~~Fix json files for sample transfer/streaming/streaming-01-http-to-http~~ fix: add missing json fields for sample streaming http-to-http Nov 7, 2024

fix: use json in test case for streaming01

d25ae74

klumbe force-pushed the fix-json branch from 2f00b9d to 5173e40 Compare November 8, 2024 09:40

ndr-brt reviewed Nov 8, 2024

View reviewed changes

fix: checksytle for Streaming01 test

ebe7946

klumbe force-pushed the fix-json branch from 5173e40 to ebe7946 Compare November 8, 2024 14:35

ndr-brt approved these changes Nov 11, 2024

View reviewed changes

ndr-brt merged commit 1d55b7c into eclipse-edc:main Nov 11, 2024
6 checks passed

ndr-brt mentioned this pull request Nov 15, 2024

Multiple errors in sample transfer/streaming/streaming-02-kafka-to-http #339

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add missing json fields for sample streaming http-to-http #335

fix: add missing json fields for sample streaming http-to-http #335

klumbe commented Nov 6, 2024 •

edited

Loading

github-actions bot left a comment

ndr-brt left a comment •

edited

Loading

klumbe commented Nov 7, 2024

ndr-brt commented Nov 7, 2024

klumbe commented Nov 7, 2024

ndr-brt commented Nov 7, 2024

klumbe commented Nov 7, 2024

klumbe commented Nov 7, 2024

klumbe commented Nov 7, 2024

ndr-brt commented Nov 8, 2024 •

edited

Loading

klumbe commented Nov 8, 2024

ndr-brt commented Nov 8, 2024

klumbe commented Nov 8, 2024

ndr-brt commented Nov 8, 2024

ndr-brt left a comment

ndr-brt Nov 8, 2024

klumbe Nov 8, 2024

klumbe commented Nov 8, 2024

ndr-brt commented Nov 11, 2024

klumbe commented Nov 11, 2024

ndr-brt commented Nov 11, 2024

ndr-brt left a comment

klumbe commented Nov 11, 2024

klumbe commented Nov 11, 2024

fix: add missing json fields for sample streaming http-to-http #335

fix: add missing json fields for sample streaming http-to-http #335

Conversation

klumbe commented Nov 6, 2024 • edited Loading

What this PR changes/adds

Why it does that

Further notes

Linked Issue(s)

github-actions bot left a comment

Choose a reason for hiding this comment

ndr-brt left a comment • edited Loading

Choose a reason for hiding this comment

klumbe commented Nov 7, 2024

ndr-brt commented Nov 7, 2024

klumbe commented Nov 7, 2024

ndr-brt commented Nov 7, 2024

klumbe commented Nov 7, 2024

klumbe commented Nov 7, 2024

klumbe commented Nov 7, 2024

ndr-brt commented Nov 8, 2024 • edited Loading

klumbe commented Nov 8, 2024

ndr-brt commented Nov 8, 2024

klumbe commented Nov 8, 2024

ndr-brt commented Nov 8, 2024

ndr-brt left a comment

Choose a reason for hiding this comment

ndr-brt Nov 8, 2024

Choose a reason for hiding this comment

klumbe Nov 8, 2024

Choose a reason for hiding this comment

klumbe commented Nov 8, 2024

ndr-brt commented Nov 11, 2024

klumbe commented Nov 11, 2024

ndr-brt commented Nov 11, 2024

ndr-brt left a comment

Choose a reason for hiding this comment

klumbe commented Nov 11, 2024

klumbe commented Nov 11, 2024

klumbe commented Nov 6, 2024 •

edited

Loading

ndr-brt left a comment •

edited

Loading

ndr-brt commented Nov 8, 2024 •

edited

Loading