Skip to content

Conversation

@ahsonkhan
Copy link
Contributor

@ahsonkhan ahsonkhan commented Nov 28, 2022

The goal is to detect issues like this and remain green by making it a part of CI:
#4130

This is similar to what we do in Embedded C:
https://github.com/Azure/azure-sdk-for-c/blob/cd62e2b78a396c3d5478d3b60c046adb72687274/eng/pipelines/templates/jobs/archetype-sdk-client.yml#L369-L390

@ahsonkhan ahsonkhan added EngSys This issue is impacting the engineering system. MQ This issue is part of a "milestone of quality" initiative. labels Nov 28, 2022
@ahsonkhan ahsonkhan added this to the 2022-12 milestone Nov 28, 2022
@ahsonkhan ahsonkhan self-assigned this Nov 28, 2022
@ahsonkhan
Copy link
Contributor Author

/azp run cpp - core - ci

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@ahsonkhan
Copy link
Contributor Author

/azp run cpp - core - ci

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

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.

LibcurlTransportAdapter.md has the character. If this catches it, we're good.

@antkmsft
Copy link
Member

antkmsft commented Nov 28, 2022

It did catch in the LibcurlTransportAdapter.md, great!

As for the errors in unit tests like test/nlohmann-json-test/src/unit-inspection.cpp:301: CHECK(json("❤️").dump() == "\"❤️\"");, if it was in our tests, I'd say we should rewrite string literals using escape characters (for example, "❤️" can be written as "\u2764\uFE0F" or as "\xE2\x9D\xA4\xEF\xB8\x8F" - they all produce the very same string with exactly the same character: strcmp("\u2764\uFE0F", "\xE2\x9D\xA4\xEF\xB8\x8F") == 0 && strcmp("❤️", "\u2764\uFE0F") == 0).

But given that it is in 3rd party code, CI probably should be ignoring errors in these files.

@ahsonkhan
Copy link
Contributor Author

Yep, that's the plan. Will filter out test recordings and nlohmann json. The main focus is product/SDK code.

@antkmsft
Copy link
Member

@ahsonkhan, if this PR is close to achieving its goal, can we get to merge it soon?

@ahsonkhan ahsonkhan marked this pull request as ready for review January 17, 2023 02:38
@ahsonkhan ahsonkhan merged commit 876a7ba into main Jan 26, 2023
@ahsonkhan ahsonkhan deleted the ahsonkhan-patch-2 branch January 26, 2023 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

EngSys This issue is impacting the engineering system. MQ This issue is part of a "milestone of quality" initiative.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants