Skip to content

Conversation

Alexander-Kiselyov
Copy link

@Alexander-Kiselyov Alexander-Kiselyov commented Aug 26, 2020

Description

Adding convenience method that allows to replace '\n' characters with the platform-dependent ones and applying it to affected test data.

Motivation and Context

Partial fix for #986. Namely for the issue described in this comment.

Testing

Re-run software.amazon.awssdk.codegen.model.intermediate.DocumentationBuilderTest and software.amazon.awssdk.codegen.emitters.PoetGeneratorTaskIntegrationTest, which were failing previously.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2020

Codecov Report

Merging #2006 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2006   +/-   ##
=========================================
  Coverage     76.92%   76.92%           
  Complexity      225      225           
=========================================
  Files          1107     1107           
  Lines         33507    33507           
  Branches       2573     2573           
=========================================
+ Hits          25774    25776    +2     
+ Misses         6453     6452    -1     
+ Partials       1280     1279    -1     
Flag Coverage Δ Complexity Δ
#unittests 76.92% <ø> (+<0.01%) 225.00 <ø> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...ty/internal/IdleConnectionCountingChannelPool.java 87.80% <0.00%> (+2.43%) 0.00% <0.00%> (ø%)

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 1fcc51d...6c94334. Read the comment docs.

@zoewangg
Copy link
Contributor

Thank you for creating the PR! 😄 We'll take a look shortly

@zoewangg
Copy link
Contributor

mvn test -pl :codegen -Dtest=DocumentationBuilderTest succeeded in aws-sdk-java-v2-JDK8-windows

Copy link
Contributor

@zoewangg zoewangg left a comment

Choose a reason for hiding this comment

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

Changes LGTM, just one minor comment on the changelog. Once it's fixed, could you do a squash and rebase? I'll merge it afterwards.

CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
# __2.14.6__
Copy link
Contributor

Choose a reason for hiding this comment

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

This changelog is actually auto generated. You can add a new entry by running the scripts/new-change script and following the instructions, and then commit the new file created by the script in .changes/next-release with your changes.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, I didn't even fully read a paragraph regarding this script in contribution guide since I thought that this is optional... Fixed this and slightly modified the guide itself to highlight the fact that script usage is mandatory (hopefully it's OK to do this in this PR and not in the scope of a separate issue).

@Alexander-Kiselyov Alexander-Kiselyov changed the title #986 Fixing DocumentationBuilderTest on Windows systems #986 Fixing codegen's unit- and integration tests on Windows systems Aug 29, 2020
@Alexander-Kiselyov
Copy link
Author

Changes LGTM, just one minor comment on the changelog. Once it's fixed, could you do a squash and rebase? I'll merge it afterwards.

Thanks for looking! Do I need to squash all the commits into one as well?

@zoewangg
Copy link
Contributor

Thanks for making the change. Yes, it'd be great if you can squash all commits into one.

Adding convenience method that allows to replace '\n' characters with the platform-dependent ones and applying it to affected test data.
Supplementing CHANGELOG.MD with changes' description. Clarifying necessity of using the new-change script for such actions.
@Alexander-Kiselyov Alexander-Kiselyov force-pushed the akiselyov/codegen-tests-cross-platform-fix branch from 7c1a7ff to 3122b10 Compare August 31, 2020 23:44
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@Alexander-Kiselyov
Copy link
Author

@zoewangg done, please take a look.

Copy link
Contributor

@zoewangg zoewangg left a comment

Choose a reason for hiding this comment

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

:shipit: Thank you for your contribution!

@zoewangg zoewangg merged commit 255e17c into aws:master Sep 1, 2020
@Alexander-Kiselyov Alexander-Kiselyov deleted the akiselyov/codegen-tests-cross-platform-fix branch September 6, 2020 02:19
aws-sdk-java-automation added a commit that referenced this pull request May 5, 2022
…55170e75f

Pull request: release <- staging/028ba70b-cdfc-49a2-85fb-71f55170e75f
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.

3 participants