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

added a test helper to test each type for having the expected default operations. #4627

Merged
merged 5 commits into from
May 26, 2023

Conversation

jnyfah
Copy link
Contributor

@jnyfah jnyfah commented May 10, 2023

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

@github-actions github-actions bot added Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels May 10, 2023
@github-actions
Copy link

Thank you for your contribution @jnyfah! We will review the pull request and get back to you soon.

@jnyfah
Copy link
Contributor Author

jnyfah commented May 10, 2023

@antkmsft Thank you for your help in clarifying this issue for me

I have added a test helper class that checks for the expected default operations for each type, but I'm not sure if I have placed the helper class in the right location and I am also running into some issues with the clang compiler. Specifically, I'm getting undefined reference errors for the type traits.

Could you take a look and let me know if you have any suggestions? Thanks !

Copy link
Member

@LarryOsterman LarryOsterman left a comment

Choose a reason for hiding this comment

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

Changes look good to me, thanks for the contribution.

Btw, the breadth of compilers we deal with makes most PRs challenging, I'll take a quick peek to see if I can figure out what's going wrong.

ETA: I suspect that the problem may be that the Args template parameter pack may be interfering with the template definition for is_assignable.

    // Check if T is assignable from U
    template<typename U>
    static constexpr bool is_assignable = std::is_assignable<T&, U>::value;

I'm not 100% sure about this, but based on my experience MSVC tends to be a lot more permissive than other compilers.

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

Thank you @jnyfah for your contribution!
This looks pretty solid. I'd love to see our SDK types covered by these tests.
How many types are you planning to cover?
In its current state, when CI succeeds, it is pretty good already as an example to copy-paste and multiply for the rest of the types, so it is up to you where you want to make a break or a stop. But even if it goes in just for the ETag type, it is atomic and it would leave our repo in a consistent state, and definitely in a better state than it was before.
The only thing we'd need to fix now is the failure of these tests:

146 - azure-core.Etag.Assignable (Failed)
	147 - azure-core.Etag.Constructible (Failed)
	148 - azure-core.Etag.CopyAndMoveConstructible (Failed)
	149 - azure-core.Etag.CopyAndMoveAssignable (Failed)
	150 - azure-core.Etag.Destructible (Failed)

I haven't looked deeply, but it looks like there is a problem with the method that we currently use to detect Destructible, for example, because I don't think ETags are not destructible, so there seems some disconnect there.

@jnyfah
Copy link
Contributor Author

jnyfah commented May 15, 2023

Thanks @LarryOsterman, figured it out, it was static constexpr, the way clang handle the instantiation is different from that of MSVC.

@jnyfah
Copy link
Contributor Author

jnyfah commented May 16, 2023

@antkmsft, fixed the tests it looks like there's another person showing interest in working on this as well. I don't want to hog all the fun by myself 🙂, so I think it would be great if we could collaborate. I would love to start with the azure.core types.

@jnyfah jnyfah marked this pull request as ready for review May 16, 2023 16:16
@antkmsft antkmsft merged commit 5499eee into Azure:main May 26, 2023
gearama added a commit that referenced this pull request May 30, 2023
* Remove -Login and -PushImages parameters from stress helper (#4615)

* Add Azure OpenAI (#4644)

Co-authored-by: Shawn Fang <[email protected]>

* move the cleanup config to -SaveInputs parameter on the cli (#4646)

Co-authored-by: m-nash <[email protected]>

* Fixes for uploading proxy logs (#4652)

Co-authored-by: Bill Wert <[email protected]>

* Handle docker hangs and subscription mismatch on acr login (#4653)

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 6202 (#4651)

* Adding initial commit of TypeSpecE2E common script

* add TODO comment

* Updated per feedback

---------

Co-authored-by: raychen <[email protected]>

* added a test helper to test each type for having the expected default operations. (#4627)

* add test helper

* constructible from Args

* restucture ClassTraits to ensure portability of static constexpr

* fix Etags tests and format

* clang format

* Add DefaultAzureCredential auth flow diagram (#4654)

* Add DefaultAzureCredential auth flow diagram

* Update cspell.json

---------

Co-authored-by: Anton Kolesnyk <[email protected]>

* Remove defunct template (#4659)

Co-authored-by: James Suplizio <[email protected]>

* Convert most AMQP types to read-only; Significant API surface cleanup. (#4640)

* Prep for readonly connection

* Added options for Connection and Session to enable read-only configuration

* Read-only connection and session objects; use message source and target for message sender and receiver

* Enabled read-only session; added tests for incorrect AMQPS port

* Added token reader sample; flushed out eventhubs properties sample, added some timeouts to send async test

* Moved AMQP samples to internal

* Moved samples to use GetEnvHelper

---------

Co-authored-by: Anton Kolesnyk <[email protected]>

* Define a standardized header ordering for Azure SDK for C++ (#4632)

* Standardized header ordering for C++ repo

* Reordered categories to move private headers above public headers; added space between categories to enable future flexibility

* fix flaky storage testcase (#4660)

---------

Co-authored-by: Ben Broderick Phillips <[email protected]>
Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Shawn Fang <[email protected]>
Co-authored-by: m-nash <[email protected]>
Co-authored-by: Bill Wert <[email protected]>
Co-authored-by: raychen <[email protected]>
Co-authored-by: Jennifer Chukwu <[email protected]>
Co-authored-by: Scott Addie <[email protected]>
Co-authored-by: Anton Kolesnyk <[email protected]>
Co-authored-by: James Suplizio <[email protected]>
Co-authored-by: Larry Osterman <[email protected]>
Co-authored-by: JinmingHu <[email protected]>
@ahsonkhan ahsonkhan mentioned this pull request May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants