Skip to content

fix: add blog post comment schema and tests #970

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
Merged
merged 5 commits into from
Dec 18, 2020
Merged

fix: add blog post comment schema and tests #970

merged 5 commits into from
Dec 18, 2020

Conversation

@lawmicha
Copy link
Contributor

@lawmicha lawmicha commented Dec 14, 2020

Issue #, if available:

Description of changes:

  • Add the blog, post, comment schema that developers can get when they run amplify add api
  • Add placeholder for API integration tests (this is in anticipation of lazy loading test for infinitely traversable data)
  • Add datastore integration test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Dec 14, 2020 •
edited
Loading

Codecov Report

Merging #970 (305c417) into main (8428e9c) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #970      +/-   ##
==========================================
+ Coverage   55.10%   55.13%   +0.02%     
==========================================
  Files         630      630              
  Lines       18195    18195              
==========================================
+ Hits        10027    10031       +4     
+ Misses       8168     8164       -4
Flag Coverage Δ
API_plugin_unit_test 51.32% <ø> (+0.13%) ⬆️
AWSPluginsCore 72.41% <ø> (ø)
Amplify 45.77% <ø> (+0.04%) ⬆️
Analytics_plugin_unit_test 73.23% <ø> (ø)
Auth_plugin_unit_test 67.43% <ø> (ø)
DataStore_plugin_unit_test 71.02% <ø> (ø)
Predictions_plugin_unit_test 33.48% <ø> (ø)
Storage_plugin_unit_test 57.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../DataStore/Model/Internal/Schema/ModelSchema.swift 85.29% <0.00%> (ø)
...CategoryPlugin/Operation/AWSGraphQLOperation.swift 63.29% <0.00%> (+2.53%) ⬆️
...s/AWSHubPlugin/Internal/HubChannelDispatcher.swift 97.05% <0.00%> (+2.94%) ⬆️
...ugins/AWSHubPlugin/Internal/SerialDispatcher.swift 88.23% <0.00%> (+5.88%) ⬆️

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 8428e9c...305c417. Read the comment docs.

@lawmicha lawmicha requested review from 618coffee and wooj2 December 14, 2020 18:06
@lawmicha lawmicha force-pushed the fix/blogpostcomment branch from 0ddd802 to a77fc97 Compare December 15, 2020 20:14
wooj2
wooj2 reviewed Dec 16, 2020
View reviewed changes
.../AWSAPICategoryPluginFunctionalTests/GraphQLModelBased/GraphQLConnectionScenario6Tests.swift
override func setUp() {
do {
Amplify.Logging.logLevel = .verbose
try Amplify.add(plugin: AWSAPIPlugin())
Copy link
Contributor

@wooj2 wooj2 Dec 15, 2020

Choose a reason for hiding this comment

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

Is there a difference between passing in the models to AWSAPIPlugin vs using ModelRegistry.register(modelType: Blog6.self)

Copy link
Contributor Author

@lawmicha lawmicha Dec 18, 2020

Choose a reason for hiding this comment

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

i don't think so, just easier to set up in the test code without having to create another class that calls the register methods

...ategoryPluginFunctionalTests/GraphQLModelBased/GraphQLConnectionScenario3Tests+Helpers.swift

extension GraphQLConnectionScenario3Tests {

func createPost(id: String = UUID().uuidString, title: String) -> Post3? {
Copy link
Contributor

@wooj2 wooj2 Dec 16, 2020

Choose a reason for hiding this comment

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

Might be better to decouple the creation of the Post3 with calling mutate. For example, something like:
func createPost(id: title) -> Post3
func mutatePost(post: Post) -> Post3

Copy link
Contributor Author

@lawmicha lawmicha Dec 18, 2020

Choose a reason for hiding this comment

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

that sounds good, mutatePost will make more sense since it's passing in some post that was retrieved instead of creating a dummy post (this was also bad because the dummy post created for the update will overwrite some other fields that take default values in the constructor)

.../AWSAPICategoryPluginFunctionalTests/GraphQLModelBased/GraphQLConnectionScenario3Tests.swift
}
}
wait(for: [completeInvoked], timeout: TestCommonConstants.networkTimeout)

Copy link
Contributor

@wooj2 wooj2 Dec 16, 2020

Choose a reason for hiding this comment

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

If we fail to wait for completeInvoked, i think the test will continue to execute here (right?)... is this what we want, or should we just fail?

Copy link
Contributor Author

@lawmicha lawmicha Dec 18, 2020

Choose a reason for hiding this comment

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

if it fails to wait for the expectation, it should fail the test and shouldn't continue to execute. we want it to continue when the expectation is met to then query for the deleted post to check that it's not there

@lawmicha lawmicha self-assigned this Dec 16, 2020
@lawmicha lawmicha added datastore Issues related to the DataStore category api Issues related to the API category labels Dec 16, 2020
618coffee
618coffee approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@618coffee 618coffee 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, you can address John's comments

wooj2
wooj2 approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@wooj2 wooj2 left a comment •
edited by lawmicha
Loading

Choose a reason for hiding this comment

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

Chatted offline, lawmicha@ will work on refactoring tests to improve readability of unit tests

@lawmicha lawmicha merged commit 492c0a3 into main Dec 18, 2020
@lawmicha lawmicha deleted the fix/blogpostcomment branch December 18, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@wooj2 wooj2 wooj2 approved these changes

@618coffee 618coffee 618coffee approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lawmicha lawmicha

Labels

api Issues related to the API category datastore Issues related to the DataStore category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lawmicha @wooj2 @618coffee