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

fix(Predictions): Fix of PredicationPlugin unit tests #903

Merged
merged 3 commits into from
Nov 24, 2020

Conversation

ruiguoamz
Copy link
Contributor

@ruiguoamz ruiguoamz commented Nov 20, 2020

Description of changes:

This PR solved several issues I found in unit tests:

  1. Added Expectation, fullfil() and wait() to every test
  2. Avoid force unwrap of error, otherwise fails the tests: https://github.com/aws-amplify/amplify-ios/pull/903/files#r527990223
  3. Properly set AWSTranscribeStreamingClientDelegate in MockTranscribeStreamingBehavior.swift: https://github.com/aws-amplify/amplify-ios/pull/903/files#diff-36fd00eb37548e02a87ce71ab27eaaf5b486a863f2350501c4059c62efc319a5R39-R40, otherwise, the checking in callback is never run.
  4. PredictionsServiceTextractTests should conforms to XCTestCase instead of XCTest otherwise, all the tests are not going to run: https://github.com/aws-amplify/amplify-ios/pull/903/files#diff-9db6d14f94733eac54f7efb37e462e3872117c41f5caf6e47da3504b2a0bc1d6R15
  5. Type of casting is incorrect: https://github.com/aws-amplify/amplify-ios/pull/903/files#diff-9db6d14f94733eac54f7efb37e462e3872117c41f5caf6e47da3504b2a0bc1d6R70

Minor things that got fixed:

  1. Formatting.
  2. Assert failure of several tests

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

@ruiguoamz ruiguoamz self-assigned this Nov 20, 2020
}
return AWSTask(error: error!)
Copy link
Contributor Author

@ruiguoamz ruiguoamz Nov 20, 2020

Choose a reason for hiding this comment

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

@ruiguoamz ruiguoamz requested review from royjit, palpatim and a team November 20, 2020 21:56
@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #903 (b8aeafc) into main (70970d2) will increase coverage by 0.90%.
The diff coverage is 87.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #903      +/-   ##
==========================================
+ Coverage   66.32%   67.22%   +0.90%     
==========================================
  Files         870      870              
  Lines       34434    34581     +147     
==========================================
+ Hits        22838    23247     +409     
+ Misses      11596    11334     -262     
Flag Coverage Δ
API_plugin_unit_test 62.56% <ø> (+0.07%) ⬆️
Analytics_plugin_unit_test 72.38% <ø> (ø)
Auth_plugin_unit_test 39.11% <ø> (ø)
DataStore_plugin_unit_test 83.03% <ø> (ø)
Predictions_plugin_unit_test 49.49% <87.75%> (+7.38%) ⬆️
Storage_plugin_unit_test 74.74% <ø> (ø)
build_test_amplify 64.30% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...uginTests/Mocks/Service/MockTextractBehavior.swift 50.00% <28.57%> (+50.00%) ⬆️
...dictionsTest/PredictionsServiceTextractTests.swift 87.35% <77.35%> (+87.35%) ⬆️
...ictionsTest/PredictionsServiceTranslateTests.swift 94.31% <89.04%> (+0.89%) ⬆️
...tionsTest/PredictionsServiceRekognitionTests.swift 87.57% <93.22%> (+2.28%) ⬆️
...ctionsTest/PredictionsServiceTranscribeTests.swift 94.21% <96.77%> (+16.76%) ⬆️
...ocks/Service/MockTranscribeStreamingBehavior.swift 75.00% <100.00%> (+25.00%) ⬆️
...ctionsTest/PredictionsServiceComprehendTests.swift 95.78% <100.00%> (+0.51%) ⬆️
...redictions/AWSPredictionsService+Rekognition.swift 57.14% <0.00%> (+0.37%) ⬆️
...Hub/DefaultPluginTests/DefaultHubPluginTests.swift 88.37% <0.00%> (+1.16%) ⬆️
...sPlugin/Support/Utils/PredictionsErrorHelper.swift 34.66% <0.00%> (+2.66%) ⬆️
... and 14 more

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 70970d2...b8aeafc. Read the comment docs.

@ruiguoamz ruiguoamz added the predictions Issues related to the Predictions category label Nov 20, 2020
@ruiguoamz ruiguoamz changed the title fix(Predictions): Fix of Predication unit tests fix(Predictions): Fix of PredicationPlugin unit tests Nov 20, 2020
let testBundle = Bundle(for: type(of: self))
guard let url = testBundle.url(forResource: "testImageLabels", withExtension: "jpg") else {
XCTFail("Unable to find image")
return
}

let errorReceived = expectation(description: "Error should be returned")
errorReceived.expectedFulfillmentCount = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this count 2? It should produce only one error right?

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, we will address this in another PR.

@@ -62,6 +64,8 @@ class PredictionsServiceTranscribeTests: XCTestCase {
resultStream.alternatives = [alternative]
results.results = [resultStream]
transcriptEvent.transcript = results
transcriptEvent.transcript?.results?.first?.isPartial = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add after line 63 alternative.isPartial = false and remove this line

Copy link
Contributor

@royjit royjit left a comment

Choose a reason for hiding this comment

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

LGTM

@ruiguoamz ruiguoamz merged commit a0d37bb into main Nov 24, 2020
@ruiguoamz ruiguoamz deleted the fix/PredictionsUnitTests branch November 24, 2020 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
predictions Issues related to the Predictions category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants