Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support OGC API - Processes - Part 1: Core #51

Merged

Conversation

sebastianfrey
Copy link
Collaborator

@sebastianfrey sebastianfrey commented Oct 30, 2023

This PR implements OGC API - Porcesses - Part 1: Core.

Implementation is basically ready, but e2e tests need some tweaking.

Closes #30.

@sebastianfrey sebastianfrey changed the title Support Requirement Support OGC API - Processes - Part 1: Core Oct 30, 2023
@sebastianfrey sebastianfrey force-pushed the feat-support-processes-api branch 2 times, most recently from 2a7bcc0 to 442d7d4 Compare October 30, 2023 15:09
@sebastianfrey
Copy link
Collaborator Author

@haoliangyu Sorry for generating so much noise today with actions. I had some problems setting up a local pygeoapi instance for e2e tests. This should be fixed now. I will create a separate PR which will add a Dockerfile, which builds a custom pygeoapi instance, which then will be used as backend for e2e tests.

@sebastianfrey sebastianfrey force-pushed the feat-support-processes-api branch 2 times, most recently from 1f3368d to a2903cc Compare October 30, 2023 20:03
@sebastianfrey
Copy link
Collaborator Author

@haoliangyu e2e tests are almost complete. I just wait for some feedback on geopython/pygeoapi#1383. I would be grateful, when you may find the time for a review, because this PR brings a lot changes with it. In other words: A second opinion does not hurt here. :-)

@haoliangyu
Copy link
Owner

@haoliangyu e2e tests are almost complete. I just wait for some feedback on geopython/pygeoapi#1383. I would be grateful, when you may find the time for a review, because this PR brings a lot changes with it. In other words: A second opinion does not hurt here. :-)

This is an important feature update and I can allocate some time this or next week for review.

@@ -27,10 +27,10 @@ This library uses the global [fetch](https://fetch.spec.whatwg.org/) function fo
``` javascript
require('isomorphic-fetch');

const { Service } = require('@ogcapi-js/features');
const { FeatureService } = require('@ogcapi-js/features');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new major version bump is required due to the name change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the name because I think FeatureService is more specific than Service. In addition it will avoid import aliases for consumers, which want to use the features and the process package together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked the latest release (0.5.0) and this breaking change has already been implemented there, but the corresponding README was not updated accordingly. This change does this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I am not aware of that change. I will still plan to release it as v1.

require('isomorphic-fetch');

const TEST_SITE = 'https://demo.ldproxy.net/vineyards';
const TEST_SITE = 'http://localhost:5000';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes how the e2e tests run. Can you provide an instruction in the packages/features/README.md on how to run e2e locally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, I will update the README.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have attached the instructions to run E2E tests locally to the root README of this library, because those instructions apply to both features and processes.

Comment on lines 77 to 96
test('e2e: getJobs() should return a jobs list', async () => {
const service = new ProcessesService({ baseUrl: TEST_SITE });
const result = await service.getJobs();

expect(Array.isArray(result.jobs)).toBe(true);
result.jobs.forEach(job => {
expect(job.processID).toBe('hello-world');
expect(job.status).toBe('successful');
});
});

test('e2e: getJob() should return a job', async () => {
const service = new ProcessesService({ baseUrl: TEST_SITE });
const result = await service.getJob('f78fa304-775f-11ee-88b4-0242ac110002');

expect(result).toBeDefined();
expect(result.processID).toBe('hello-world');
expect(result.status).toBe('successful');
expect(result.jobID).toBe('f78fa304-775f-11ee-88b4-0242ac110002');
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does these two e2e tests depend on the previous executeJob e2e test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they do not depend on the previous executeJob() tests, because the e2e docker image contains a pre-populated jobs database.

But the assertions inside the getJobs() for loop should be less specific. I will improve this test.

Copy link
Collaborator Author

@sebastianfrey sebastianfrey Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are improved.

@sebastianfrey
Copy link
Collaborator Author

@haoliangyu requested changes are implemented.

Copy link
Owner

@haoliangyu haoliangyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@haoliangyu haoliangyu merged commit fb3fe4a into haoliangyu:master Nov 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Processes API
2 participants