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

Fixes API responses #1080

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Fixes API responses #1080

merged 2 commits into from
Sep 14, 2023

Conversation

zachgk
Copy link
Contributor

@zachgk zachgk commented Sep 14, 2023

This is an update to fix the changes to the workflow response from #1022. Before, we had no test that checks the response components decided by the names of Java response object properties (workflows, models, etc.). This adds some new JSON testing to help ensure that these are not accidentally changed, fixing this issue and hopefully avoiding future breakages.

This is an update to fix the changes to the workflow response from
deepjavalibrary#1022. Before, we had no test
that checks the response components decided by the names of Java response object
properties (workflows, models, etc.). This adds some new JSON testing to help
ensure that these are not accidentally changed, fixing this issue and hopefully
avoiding future breakages.
@zachgk zachgk marked this pull request as ready for review September 14, 2023 18:28
@zachgk zachgk requested review from frankfliu and a team as code owners September 14, 2023 18:28
@@ -596,6 +600,16 @@ private void testListModels(Channel channel) throws InterruptedException {
.anyMatch(w -> expectedModel.equals(w.getModelName())));
}

// Test pure JSON works
Map<String, Object> rawResult = JsonUtils.GSON.fromJson(result, Map.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

the result has been deserialized into resp object, can we just assert each fields are not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that helps. The issue was that I changed the property name of the list from models to wpcs. This changes both the serialization in the code and deserialization in the test, so no explicit changes were necessary in the test. I wanted to use something more raw so that the names of the fields explicitly show up in our testing

@zachgk zachgk merged commit 99dd256 into deepjavalibrary:master Sep 14, 2023
7 checks passed
@zachgk zachgk deleted the responseFixed branch September 14, 2023 20:33
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.

2 participants