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

chore: Include more tests into the pipeline #1566

Merged
merged 9 commits into from
Oct 31, 2021

Conversation

mykola-mokhnach
Copy link
Contributor

Change list

Some tests exist but never get executed by the CI.

Types of changes

  • No changes in production code.
  • Bugfix (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 not work as expected)

jdkArchitectureOption: 'x64'
publishJUnitResults: true
tasks: 'build'
options: 'miscTest -x checkstyleTest -x test -x signMavenJavaPublication'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
options: 'miscTest -x checkstyleTest -x test -x signMavenJavaPublication'
options: 'miscTest -x checkstyleTest -x test'

it seems exclusion of signMavenJavaPublication is not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was not sure about this one. How do you see it's not needed?

Copy link
Collaborator

@valfirst valfirst Oct 29, 2021

Choose a reason for hiding this comment

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

I'm running it locally without exclusion and it doesn't fail

Copy link
Collaborator

Choose a reason for hiding this comment

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

but CI could differ, so it's just my assumption

Copy link
Collaborator

Choose a reason for hiding this comment

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

Created PR to check the assumption: #1568, all builds are passed.

@mykola-mokhnach mykola-mokhnach changed the title [WIP] chore: Include more tests into the pipeline chore: Include more tests into the pipeline Oct 29, 2021
@mykola-mokhnach mykola-mokhnach changed the title chore: Include more tests into the pipeline [WIP] chore: Include more tests into the pipeline Oct 29, 2021
@mykola-mokhnach mykola-mokhnach changed the title [WIP] chore: Include more tests into the pipeline chore: Include more tests into the pipeline Oct 29, 2021
@@ -33,6 +35,11 @@ public BaseMapOptionData(Map<String, Object> options) {
this.options = options;
}

public BaseMapOptionData(String json) {
//noinspection unchecked
this((Map<String, Object>) new Gson().fromJson(json, Map.class));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it would be better to convert Gson object to a static field and reuse it here and in methods toJson() and toString()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Test
public void canBuildXcuiTestOptions() throws MalformedURLException {
XCUITestOptions options = new XCUITestOptions();
assertEquals(options.getPlatformName(), Platform.IOS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

asssertEquals accepts expected as the first parameter and actual as the second parameter

Copy link
Collaborator

Choose a reason for hiding this comment

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

and it seems it's relevant to all assertions in this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@mykola-mokhnach mykola-mokhnach merged commit c3e8916 into appium:master Oct 31, 2021
@mykola-mokhnach mykola-mokhnach deleted the align_tests branch October 31, 2021 12:03
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.

None yet

3 participants