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

feat(lib): exposed testing matchers for other languages properly #1935

Merged
merged 22 commits into from
Jul 26, 2022

Conversation

Maed223
Copy link
Contributor

@Maed223 Maed223 commented Jul 19, 2022

Testing matchers placed in class for jsii exposure

Copy link
Contributor

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

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

Some smaller things

test/csharp/testing-matchers/test.ts Show resolved Hide resolved
test/csharp/testing-matchers/test.ts Outdated Show resolved Hide resolved
test/csharp/testing-matchers/test.ts Outdated Show resolved Hide resolved
@Maed223 Maed223 marked this pull request as draft July 21, 2022 20:26
@Maed223 Maed223 added the ci/updating-snapshots CI is currently updating this PR with new snapshot tests label Jul 22, 2022
@github-actions github-actions bot removed the ci/updating-snapshots CI is currently updating this PR with new snapshot tests label Jul 22, 2022
@Maed223 Maed223 added ci/update-snapshots When set on a PR a GH Action will update and push all snapshot tests ci/updating-snapshots CI is currently updating this PR with new snapshot tests and removed ci/updating-snapshots CI is currently updating this PR with new snapshot tests labels Jul 22, 2022
@github-actions github-actions bot added ci/updating-snapshots CI is currently updating this PR with new snapshot tests and removed ci/update-snapshots When set on a PR a GH Action will update and push all snapshot tests labels Jul 22, 2022
@github-actions github-actions bot removed the ci/updating-snapshots CI is currently updating this PR with new snapshot tests label Jul 22, 2022
@Maed223 Maed223 added ci/update-snapshots When set on a PR a GH Action will update and push all snapshot tests ci/updating-snapshots CI is currently updating this PR with new snapshot tests and removed ci/update-snapshots When set on a PR a GH Action will update and push all snapshot tests labels Jul 22, 2022
@github-actions github-actions bot removed the ci/updating-snapshots CI is currently updating this PR with new snapshot tests label Jul 22, 2022
@Maed223 Maed223 added ci/update-snapshots When set on a PR a GH Action will update and push all snapshot tests ci/updating-snapshots CI is currently updating this PR with new snapshot tests labels Jul 22, 2022
@github-actions github-actions bot removed the ci/update-snapshots When set on a PR a GH Action will update and push all snapshot tests label Jul 22, 2022
@Maed223 Maed223 removed the ci/updating-snapshots CI is currently updating this PR with new snapshot tests label Jul 22, 2022
Copy link
Collaborator

@jsteinich jsteinich left a comment

Choose a reason for hiding this comment

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

I mostly focused on the C# and Java pieces since I'm not familiar with unit testing in Python or Go.
Really just minor points about trying to align closer with conventions of the specific languages.

packages/cdktf-cli/templates/csharp/TestProgram.cs Outdated Show resolved Hide resolved
// The tests below are example tests, you can find more information at
// https://cdk.tf/testing
public class TestProgram{
private static TerraformStack stack = new TerraformStack(Testing.app(), "stack");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably opt for doing this in a test setup rather than a static variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do a follow up for this one 👍

packages/cdktf-cli/templates/java/pom.xml Show resolved Hide resolved
packages/cdktf-cli/templates/java/pom.xml Outdated Show resolved Hide resolved
"src/main/java/com/mycompany/app/JunitTesting.java"
);
await driver.copyFiles("update-pom.sh");
await driver.exec("bash update-pom.sh");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just copy an updated pom file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is something we can do in a follow up PR

website/docs/cdktf/test/unit-tests.mdx Show resolved Hide resolved
website/docs/cdktf/test/unit-tests.mdx Outdated Show resolved Hide resolved
website/docs/cdktf/test/unit-tests.mdx Show resolved Hide resolved
website/docs/cdktf/test/unit-tests.mdx Outdated Show resolved Hide resolved
@jsteinich
Copy link
Collaborator

In the future the creation of a testing adapter similar to the one for jest would be an improvement. Currently Users have to access the pass attribute directly, and the message attribute is more jest specific. Further refinement in the future could improve usability.

The problem I see with delaying changes is that we are likely to encounter breaking changes or further suboptimal experience when adding in the future.
Since the additional return information seems to be targeted towards jest at this point, and since jest has an additional adapter, I'd recommend having internal functions that both the jest adapter and public methods make use of. This would still allow the same code and increased jest support to be present, while simplifying the public signatures for other languages to just return a boolean.

Co-authored-by: Jon Steinich <[email protected]>
@DanielMSchmidt DanielMSchmidt dismissed laurapacilio’s stale review July 26, 2022 12:31

Outdate and we can improve the docs even after the release, so not blocking :)

@DanielMSchmidt DanielMSchmidt changed the title fix(lib): exposed testing matchers for other languages properly feat(lib): exposed testing matchers for other languages properly Jul 26, 2022
@DanielMSchmidt DanielMSchmidt merged commit abc1596 into main Jul 26, 2022
@DanielMSchmidt DanielMSchmidt deleted the fix-testing-matchers branch July 26, 2022 13:57
DanielMSchmidt added a commit that referenced this pull request Jul 28, 2022
* fix(lib): exposed testing matchers for other languages properly

* added documentation for increased support of unit testing

* updated templates to add testing and various small changes to integration tests

* fixed docs formating

* Update website/docs/cdktf/test/unit-tests.mdx

Co-authored-by: Laura Pacilio <[email protected]>

* Update website/docs/cdktf/test/unit-tests.mdx

Co-authored-by: Laura Pacilio <[email protected]>

* Update website/docs/cdktf/test/unit-tests.mdx

Co-authored-by: Laura Pacilio <[email protected]>

* Update website/docs/cdktf/test/unit-tests.mdx

Co-authored-by: Laura Pacilio <[email protected]>

* Revert "Update website/docs/cdktf/test/unit-tests.mdx"

This reverts commit 1258245.

* Revert "Update website/docs/cdktf/test/unit-tests.mdx"

This reverts commit a9cc009.

* fixed templates that caused build errors in tests

* update to testing docs

* fixed async tests for java dotnet

* chore: fix wording

Co-authored-by: Jon Steinich <[email protected]>

* chore: improve wording

Co-authored-by: Jon Steinich <[email protected]>

* chore: fix wording

Co-authored-by: Jon Steinich <[email protected]>

* chore: use default test path in java

* chore: only expose boolean instead of AssertionReturn for now

Co-authored-by: Laura Pacilio <[email protected]>
Co-authored-by: Daniel Schmidt <[email protected]>
Co-authored-by: Jon Steinich <[email protected]>
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants