Skip to content

[AMSDK-11410] Add support for configurable testing endpoints#189

Merged
nporter-adbe merged 4 commits intoadobe:devfrom
nporter-adbe:AMSDK-11410
Apr 17, 2021
Merged

[AMSDK-11410] Add support for configurable testing endpoints#189
nporter-adbe merged 4 commits intoadobe:devfrom
nporter-adbe:AMSDK-11410

Conversation

@nporter-adbe
Copy link
Copy Markdown
Contributor

@nporter-adbe nporter-adbe commented Apr 15, 2021

Description

Implements: #84

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2021

Codecov Report

Merging #189 (87e361d) into dev (4090157) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #189      +/-   ##
==========================================
+ Coverage   94.18%   94.29%   +0.11%     
==========================================
  Files          25       26       +1     
  Lines         893      910      +17     
==========================================
+ Hits          841      858      +17     
  Misses         52       52              

Copy link
Copy Markdown
Contributor

@kevinlind kevinlind 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.

@nporter-adbe nporter-adbe linked an issue Apr 16, 2021 that may be closed by this pull request
private let exEdgeInteractUrl = URL(string: FunctionalTestConst.EX_EDGE_INTERACT_URL_STR)! // swiftlint:disable:this force_unwrapping
private let exEdgeInteractProdUrl = URL(string: FunctionalTestConst.EX_EDGE_INTERACT_PROD_URL_STR)! // swiftlint:disable:this force_unwrapping
private let exEdgeInteractPreProdUrl = URL(string: FunctionalTestConst.EX_EDGE_INTERACT_PRE_PROD_URL_STR)! // swiftlint:disable:this force_unwrapping
private let exEdgeInteractIntegrationUrl = URL(string: FunctionalTestConst.EX_EDGE_INTERACT_INTEGRATION_URL_STR)! // swiftlint:disable:this force_unwrapping
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we disable this rule in Tests/TestUtils/.swiftlint.yml ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would like to disable that rule, but I'd prefer to do it in a dedicated task/PR so that we can go through all the newly disabled rules and remove all the comment annotations throughout all our tests. Otherwise, I think we'll end up with many old unneeded swiftlint:disable comments.

I can create a task if you'd like.

Copy link
Copy Markdown
Contributor

@emdobrin emdobrin left a comment

Choose a reason for hiding this comment

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

Left a small comment, looks good otherwise

}

let edgeEndpointStr = configSharedState[EdgeConstants.SharedState.Configuration.EDGE_ENVIRONMENT] as? String
let edgeEndpoint = EdgeEndpoint(optionalRawValue: edgeEndpointStr?.lowercased())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we move the lowercased inside EdgeEndpoint.init too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't mind lowercasing what we read from configuration, but I don't believe the actual enum should be case insensitive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The main reason being is I don't think an enum case should match many different values, rather only the explicit string it defines.

The following behavior I'd also find undesired:

let rawValue = "INT"
let endpoint = EdgeEndpoint(optionalRawValue: rawValue)

let equal = endpoint.rawValue == rawValue // false

@nporter-adbe nporter-adbe merged commit b1f1c7c into adobe:dev Apr 17, 2021
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.

Configurable Edge endpoint for development testing

3 participants