Skip to content

Small fixes#5122

Open
drebelsky wants to merge 9 commits intostellar:masterfrom
drebelsky:small-fixes
Open

Small fixes#5122
drebelsky wants to merge 9 commits intostellar:masterfrom
drebelsky:small-fixes

Conversation

@drebelsky
Copy link
Contributor

@drebelsky drebelsky commented Feb 3, 2026

  • Add tracy binaries to .gitignore
  • Add check for SecretKey::random in tests
  • Remove duplicated header guard
  • Don't run Soroban tests when Catch2 tests fail
  • Remove outdated documentation (Remove support for old V1 survey #4660 removed the "surveytopology" endpoint)
  • Remove impossible condition (Remove impossible condition #5119)
  • Initialize analyzeCriticalGroups cli option
  • Add clang-format rule for east const

Copilot AI review requested due to automatic review settings February 3, 2026 19:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR includes several small miscellaneous fixes to improve the development workflow and test infrastructure.

Changes:

  • Adds tracy profiling tool binaries to .gitignore
  • Adds static check to prevent use of SecretKey::random() in test files
  • Removes redundant #pragma once directive from basen.h header (which already has include guards)
  • Attempts to modify CI to skip Soroban tests when Catch2 tests fail

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
.gitignore Adds tracy-capture and tracy-gui binaries to ignore list
src/test/check-nondet Adds check to prevent non-deterministic SecretKey::random() usage in tests
lib/util/basen.h Removes duplicate #pragma once (file uses traditional include guards)
ci-build.sh Adds TESTS_ENVIRONMENT to skip tests after failures (has implementation issues)

@drebelsky
Copy link
Contributor Author

drebelsky commented Feb 3, 2026

Putting some notes on the CI modification. This approach seems to work, although it does feel somewhat hacky (especially since it's relying on the internal automake implementation of serialized tests using a failed variable). In particular, I don't think it will handle stopping the rest of the tests if one of the recursive lib tests fails (although, if the lib tests are run serially, it will stop that serial run). Additionally, since the order of the tests in src is selftest-{no}pg, check-nondet, check-sorobans, we get the odd behavior where failing check-nondet stops check-sorobans from running (but not selftest). I'm happy to remove or rewrite the commit based on what others think, but I'm leaning toward wrapping selftest-{no}pg and check-sorobans into a wrapper script so that check-sorobans only runs when selftest passes (leaving the rest of the behavior as it was pre this PR).

@bboston7
Copy link
Contributor

I'm leaning toward wrapping selftest-{no}pg and check-sorobans into a wrapper script so that check-sorobans only runs when selftest passes (leaving the rest of the behavior as it was pre this PR).

I like this option. I agree that depending on automake internals is not ideal.

// The version of upgrade parameters serialization that introduced the
// nominationtimeoutlimit and expirationminutes fields.
constexpr const uint32_t UPGRADE_VERSION_WITH_NOMINATION_STRIPPING = 1;
constexpr uint32_t const UPGRADE_VERSION_WITH_NOMINATION_STRIPPING = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe also worth adding QualifierOrder so constexpr also appears to the right (assuming that's what we want)

Copy link
Contributor

Choose a reason for hiding this comment

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

Weirdly enough I think west constexpr is way more common across our codebase. I really don't have an opinion about that, and would gladly follow whatever someone who feels more strongly about this prefers (including doing nothing).

That being said, the const on this line and on line 43 are unnecessary and implied by the constexpr. Maybe worth cleaning up since you're modifying these lines anyway.

Copy link
Contributor

@bboston7 bboston7 left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I think this is looking great except for the exec issue.

// The version of upgrade parameters serialization that introduced the
// nominationtimeoutlimit and expirationminutes fields.
constexpr const uint32_t UPGRADE_VERSION_WITH_NOMINATION_STRIPPING = 1;
constexpr uint32_t const UPGRADE_VERSION_WITH_NOMINATION_STRIPPING = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Weirdly enough I think west constexpr is way more common across our codebase. I really don't have an opinion about that, and would gladly follow whatever someone who feels more strongly about this prefers (including doing nothing).

That being said, the const on this line and on line 43 are unnecessary and implied by the constexpr. Maybe worth cleaning up since you're modifying these lines anyway.

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