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.

Sign up for GitHub

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

Jump to bottom

Lazify cloud request engine properties #25

Merged
merged 6 commits into from
Oct 14, 2023
Merged

Lazify cloud request engine properties #25

merged 6 commits into from
Oct 14, 2023

Conversation

Riabkov
Copy link
Contributor

@Riabkov Riabkov commented Oct 9, 2023

No description provided.

@justadreamer
Copy link
Contributor

We also need to verify a couple more scenarios with unit/integration tests. Please use mocking/injection techniques to write these tests. We need to simulate the failing cloud call (a call to cloud.51degrees.com)

  1. If the call to the cloud fails - we should not get an exception on pipeline construction, for the properties are now lazy, but should get it when we call Process() function
  2. if we construct the pipeline with the SuppressProcessExceptions=true, we should not get the exceptions when cloud.51degrees.com is unavailable, but they should be logged and added into the flowData.errors

justadreamer
justadreamer requested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@justadreamer justadreamer left a comment

Choose a reason for hiding this comment

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

a couple changes in code (to prevent race conditions) + a couple unhappy path scenario tests need to be added

justadreamer
justadreamer requested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@justadreamer justadreamer left a comment

Choose a reason for hiding this comment

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

a couple more amendments to the tests would be nice

...src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTests.java Show resolved Hide resolved
...src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTests.java Outdated Show resolved Hide resolved
@Riabkov Riabkov requested a review from justadreamer October 11, 2023 13:36
@Automation51D Automation51D merged commit f20771b into 51Degrees:main Oct 14, 2023
@justadreamer justadreamer mentioned this pull request Oct 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justadreamer justadreamer justadreamer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Riabkov @justadreamer @Automation51D