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

Use centralised assert.sh script [DI-171] #206

Conversation

JackPGreen
Copy link
Collaborator

@JackPGreen JackPGreen commented Jul 8, 2024

The assert.sh script was sourced externally but a local copy was included in the repository.

This change removes the local copy, instead dynamically fetching from a central Hazelcast fork.

Our local copy had some small changes, which have been lost, but the previous version has been migrated to a branch of the Hazelcast fork to allow a diff of the script.

Specifically, the changes are only to improve formatting of assertion messages, not the assertions themselves.

Fixes: DI-171

The `assert.sh` script was [sourced externally](https://github.com/torokmark/assert.sh) but a local copy was included in the repository.

This change removes the local copy, instead dynamically fetching from a central [Hazelcast fork](https://github.com/hazelcast/assert.sh).

Our local copy had some small changes, which have been lost, but the previous version has been migrated to a branch of the Hazelcast fork to allow [a diff of the script](hazelcast/assert.sh@main...hazelcast-packaging).

Specifically, the changes are only to improve formatting of assertion messages, not the assertions themselves.

Fixes: [DI-171](https://hazelcast.atlassian.net/browse/DI-171)
@JackPGreen JackPGreen self-assigned this Jul 8, 2024
@JackPGreen
Copy link
Collaborator Author

@ldziedziul @nishaatr are you able to take a look at this, please?

Copy link
Contributor

@nishaatr nishaatr left a comment

Choose a reason for hiding this comment

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

LGTM at at back of hazelcast/hazelcast-docker#779

@ldziedziul
Copy link
Contributor

Waiting with approval update of logging in tests

@JackPGreen
Copy link
Collaborator Author

Waiting with approval update of logging in tests

Done.

@@ -11,7 +14,8 @@ function assertAlphanumCamelCase {
local testValue=$1
local expected=$2
local actual=$(alphanumCamelCase "$testValue")
assert_eq "$expected" "$actual" "Alphanumeric camel case of $testValue should be equal to $expected " || TESTS_RESULT=$?
local msg="Alphanumeric camel case of $testValue should be equal to $expected"
assert_eq "$expected" "$actual" "$msg" && log_success "$MSG" || TESTS_RESULT=$?
Copy link
Contributor

Choose a reason for hiding this comment

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

Casing is mixed up here and this file is not run during be test_scripts.sh. Could you rename this file brew_functions_test.sh to be picked up ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Casing is mixed up here

I'll fix that

8c8c7ae

and this file is not run during be test_scripts.sh. Could you rename this file brew_functions_test.sh to be picked up ?

I've tried that, but unfortunately the script doesn't pass on master either. So for now I'll leave it alone, but raise as a separate issue for the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...actually this might be a Mac + sed issue. Let me take another look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

8547cd0

Renaming them works on non-Mac platforms.

@ldziedziul
Copy link
Contributor

@JackPGreen
Copy link
Collaborator Author

There's some issue here: https://github.com/hazelcast/hazelcast-packaging/actions/runs/10187951924/job/28207888467#step:4:36 image

The action was running on the merge commit, as that test is new and wasn't in this branch... fixed.

8f66e41

@JackPGreen JackPGreen merged commit 2a41e73 into master Aug 1, 2024
8 checks passed
@JackPGreen JackPGreen deleted the DI-171---Migrate-workflows-to-use-assert.sh-from-a-single-repository branch August 1, 2024 12:55
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