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

Refactoring and adding aws_sdk_unstable to RUSTFLAG on one of the kotlin test #2804

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

thomas-k-cameron
Copy link
Contributor

Motivation and Context

This is a sub-PR of #2615

It adds aws_sdk_unstable flag to one of the kotlin test.

Description

  1. adds --cfg aws_sdk_unstable to codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/Rust.kt
  2. Refactors the code in the file by creating a variable that holds hard-coded values.

Testing

NA

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@thomas-k-cameron thomas-k-cameron requested review from a team as code owners June 22, 2023 08:49
@thomas-k-cameron thomas-k-cameron mentioned this pull request Jun 22, 2023
2 tasks
thomas-k-cameron and others added 2 commits June 23, 2023 01:57
…en/core/testutil/Rust.kt

Co-authored-by: Zelda Hessler <zelda.hessler@pm.me>
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

I think we'll lose something by making all tests run with --cfg aws_sdk_unstable. Specifically, we won't be able to unit test without that enabled anymore.

Rather than apply it to everything, we should make it possible to add additional compiler flags when calling compileAndTest, and then update some key/relevant unit tests to test it both ways.

@thomas-k-cameron
Copy link
Contributor Author

thomas-k-cameron commented Jun 29, 2023

Ok, I made the unstable features optional.

Features are enabled by default, but it can be turned off by passing enableUnstableFeature = False.

thomas-k-cameron and others added 5 commits June 30, 2023 02:38
…en/core/testutil/Rust.kt

Co-authored-by: John DiSanti <johndisanti@gmail.com>
…en/core/testutil/Rust.kt

Co-authored-by: John DiSanti <johndisanti@gmail.com>
@thomas-k-cameron
Copy link
Contributor Author

I think it's ready.

private const val allFeature = "--all-features"

fun cargoTest(enableUnstable: Boolean): String {
return func("cargo test", allFeature, enableUnstable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this isn't quite doing what is expected by the enableUnstable flag since it adds --all-features rather than the individual features.

Copy link
Contributor Author

@thomas-k-cameron thomas-k-cameron Jul 4, 2023

Choose a reason for hiding this comment

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

Since only features that can be enabled are protected behind feature gate, I thought it was accurate. I changed the name to enableAllFeatures

I also added a function that accepts array of string as an argument; In this case, you can specified individual features.
e.g.

// enable features specified in the array
// e.g.
// ```kotlin
// cargoCheck(["serde-serialize", "serde-deserialize"])
// // cargo check --features serde-serialize serde-deserialize
// ```
fun cargoCheck(featuresToEnable?: Array<String>): String {
    return func("cargo check", allFeature, enableAllFeatures)
}

@thomas-k-cameron
Copy link
Contributor Author

So i got an error:

failures:

---- tls::listener::tests::server_doesnt_shutdown_after_bad_handshake stdout ----
thread 'tls::listener::tests::server_doesnt_shutdown_after_bad_handshake' panicked at 'assertion failed: response.unwrap_err().to_string().contains(\"invalid peer certificate: UnknownIssuer\")', aws-smithy-http-server-python/src/tls/listener.rs:144:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Is this something to do with the connection?

Velfi
Velfi previously approved these changes Jul 6, 2023
@thomas-k-cameron
Copy link
Contributor Author

Let's run the test again.

@thomas-k-cameron
Copy link
Contributor Author

thomas-k-cameron commented Jul 6, 2023

It seems like I'm not the only one running into invalid peer certificate: UnknownIssuer problem.

https://github.com/awslabs/smithy-rs/actions/runs/5476687598/jobs/9975777528?pr=2829

running 24 tests
test middleware::header_map::tests::py_header_map ... ok
test socket::tests::socket_can_be_cloned ... ok
test socket::tests::socket_can_bind_on_random_port ... ok
test lambda::tests::py_lambda_context ... ok
test tls::listener::tests::server_doesnt_shutdown_after_bad_handshake ... FAILED
test tls::listener::tests::server_shutdown_after_listener_error - should panic ... ok
test tls::tests::creating_tls_config_in_python ... ok
test tls::listener::tests::server_changes_tls_config ... ok
test context::tests::py_context ... ok
test context::tests::works_with_none ... ok
test context::lambda::tests::works_with_none ... ok
test context::layer::tests::populates_lambda_context ... ok
test context::lambda::tests::py_context_with_lambda_context ... ok
test util::collection::tests::mutable_mapping ... ok
test util::error::tests::rich_python_errors ... ok
test util::tests::check_if_is_optional_of ... ok
test util::tests::function_metadata ... ok
test logging::tests::tracing_handler_is_injected_in_python ... ok
test types::tests::blob_can_be_used_in_python_when_initialized_in_rust ... ok
test types::tests::blob_can_be_initialized_in_python ... ok
test types::tests::bytestream_can_be_used_in_sync_python_when_initialized_in_rust ... ok
test types::tests::datetime_can_be_used_in_python_when_initialized_in_rust ... ok
test types::tests::document_type ... ok
test types::tests::datetime_can_by_initialized_in_python ... ok

failures:

---- tls::listener::tests::server_doesnt_shutdown_after_bad_handshake stdout ----
thread 'tls::listener::tests::server_doesnt_shutdown_after_bad_handshake' panicked at 'assertion failed: response.unwrap_err().to_string().contains(\"invalid peer certificate: UnknownIssuer\")', aws-smithy-http-server-python/src/tls/listener.rs:144:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@thomas-k-cameron
Copy link
Contributor Author

#2804 (comment)
#2804 (comment)

It's fixed.

@rcoh rcoh requested review from a team as code owners November 14, 2023 02:21
} catch (e: Exception) {
// cargo fmt errors are useless, ignore
}


// Clean `RUSTFLAGS` because in CI we pass in `--deny warnings` and
// we still generate test code with warnings.
// TODO(https://github.com/smithy-lang/smithy-rs/issues/3194)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove this comment?

I added a function that create a map which cleans up rustflag as well.

@thomas-k-cameron
Copy link
Contributor Author

all fixed.

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