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

Add ANTITHESIS_MAX_NAME_LEN macro #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bronek
Copy link
Contributor

@Bronek Bronek commented Dec 16, 2024

We have new users defining their own instrumentation points, and they have started using some very long messages, unaware that this may cause readability issues in report and also that these names are stuck "forever" (or until report history is truncated).

This change will enable us to enforce maximum assertion message (name) length.

I tested this with

#define ANTITHESIS_MAX_NAME_LEN 9

#include "antithesis_sdk.h"

int main() {
  using namespace antithesis;
  REACHABLE("Message 1", {{"a", 1}});
  ALWAYS_OR_UNREACHABLE(false, "Message 2");
  ALWAYS_GREATER_THAN(10, 20, "Hello");
  ALWAYS_SOME(NAMED_LIST(false), "Message");
  SOMETIMES_ALL(NAMED_LIST(false), "Msg");
  return 0;
}

I am not planning to bring this functionality to polyfills, as it would bring lots of extra assumptions into what polyfills are allowed (or not) to do.

This enables the user to enforce maximum assertion message (name) length
@@ -767,6 +767,8 @@ namespace antithesis::internal::assertions {

namespace antithesis::internal {
namespace { // Anonymous namespace which is translation-unit-specific; certain symbols aren't exposed in the symbol table as a result
enum msg_name_e { msg_name };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggestions for better name here are welcome.

@wsx-antithesis
Copy link
Collaborator

We have new users defining their own instrumentation points, and they have started using some very long messages, unaware that this may cause readability issues in report and also that these names are stuck "forever" (or until report history is truncated).

I agree there are some conventions that one should follow in assertion messages to provide a good experience,
but we're not sure yet if imposing a user-configurable length limit is the way to go here.

This is still great feedback, and we'll definitely want to improve our documentation to help people follow good conventions in authoring assertions.
We just need to think a bit harder on whether to have some compile-time enforceable rules that works across all of our SDKs.

I'll keep this open for new ideas!

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.

2 participants