Skip to content

fix: add Optimizely builder option for client-engine info#466

Merged
jaeopt merged 6 commits into
masterfrom
jae/event-version
Mar 15, 2022
Merged

fix: add Optimizely builder option for client-engine info#466
jaeopt merged 6 commits into
masterfrom
jae/event-version

Conversation

@jaeopt

@jaeopt jaeopt commented Mar 9, 2022

Copy link
Copy Markdown
Contributor

Summary

  • Add an option to specify client-engine version (android-sdk, etc) in the Optimizely builder.

Test plan

  • Add a unit test for builder with client-engine version.

Issues

  • OASIS-8188

@jaeopt jaeopt requested a review from a team as a code owner March 9, 2022 18:05
@coveralls

coveralls commented Mar 9, 2022

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1965

  • 12 of 12 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 90.806%

Totals Coverage Status
Change from base Build 1962: 0.05%
Covered Lines: 4889
Relevant Lines: 5384

💛 - Coveralls

Comment thread core-api/src/main/java/com/optimizely/ab/Optimizely.java

// can be overridden by other wrapper client (android-sdk, etc)

private static String clientVersion = readVersionNumber();

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.

final?

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.

Unlike the old VERSION, this private variable can be changed with Optimizely.Builder.


eventHandler = mock(EventHandler.class);
optimizely = Optimizely.builder(validConfigJsonV4(), eventHandler)
.withClientInfo(EventBatch.ClientEngine.ANDROID_SDK, "1.2.3")

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.

isn't it confusing to check "android-sdk" client engine.

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.

Can you clarify this comment? Not clear to me.

@msohailhussain msohailhussain left a comment

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.

lgtm

@jaeopt jaeopt merged commit 336b610 into master Mar 15, 2022
@jaeopt jaeopt deleted the jae/event-version branch March 15, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants