Skip to content

Conversation

@Ducasse
Copy link
Member

@Ducasse Ducasse commented Nov 15, 2025

No description provided.

@request-info
Copy link

request-info bot commented Nov 15, 2025

This issue has either a default title or empty body. We would appreciate it if you could provide more information. Note: I am not a very intelligent bot, I can only react to new comments. Please add a comment for me if you update the body or title.

@jecisc
Copy link
Member

jecisc commented Nov 17, 2025

It has been 2 build that command line tests are not passing. This seems related since it passed on other PRs

@Ducasse
Copy link
Member Author

Ducasse commented Nov 18, 2025

I do not get what you are saying.

[Tests](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-18906/2/testReport/) (3 failures / -1)
[osx-64 / Tests-osx-64 / MacOSX64.ReleaseTests.ReleaseTest.testMethodsContainNoHalt](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-18906/2/testReport/junit/MacOSX64.ReleaseTests/ReleaseTest/osx_64___Tests_osx_64___testMethodsContainNoHalt/)
[unix-64 / Tests-unix-64 / Unix64.ReleaseTests.ReleaseTest.testMethodsContainNoHalt](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-18906/2/testReport/junit/Unix64.ReleaseTests/ReleaseTest/unix_64___Tests_unix_64___testMethodsContainNoHalt/)
[windows-64 / Tests-windows-64 / Windows64.ReleaseTests.ReleaseTest.testMethodsContainNoHalt](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-18906/2/testReport/junit/Windows64.ReleaseTests/ReleaseTest/windows_64___Tests_windows_64___testMethodsContainNoHalt/)

@Ducasse
Copy link
Member Author

Ducasse commented Nov 18, 2025

I will not fix what I do not understand. Sorry but I cannot.
I do not even know the code and this is probably bash code.
@demarey may be you know.
For me I will merge the PR since Pharo tests are green.

ok 40 test can run tests on a package # in 405 ms
not ok 41 test can run tests on a regex # in 572 ms
# (from function `__assert_line' in file tests/vendor/bats-assert/src/assert_line.bash, line 298,
#  from function `assert_line' in file tests/vendor/bats-assert/src/assert_line.bash, line 131,
#  in test file tests/pharo-cli-test-runner.bats, line 30)
#   `assert_line "Running tests in 2 Packages"' failed
# 
# -- output does not contain line --
# line : Running tests in 2 Packages
# output (39 lines):
#   Beginning to run tests of SUnit-UI-Tests with random seed 239983797

@jecisc
Copy link
Member

jecisc commented Nov 18, 2025

Those are functional tests of the pharo command line handling.

It's tests running some command lines and checking that the output of the tests are those expected.
They cannot be written as pharo tests sadly :/

@jecisc
Copy link
Member

jecisc commented Nov 18, 2025

I might have an idea of the origin of the error.

With the InformativeNotification, if NewTools is in the image, we ask it to inform the user via growl (in the morphic adapter).

But if the image is not interactive, then we do nothing while we should print the string in the stdout!

I'll try to fix this tomorrow if the error is this one (it's just a hunch for now)

Revert SUnit-Basic-Clap because superclass defines an inform: method
@Ducasse
Copy link
Member Author

Ducasse commented Nov 18, 2025

let us see.

@demarey
Copy link
Contributor

demarey commented Nov 19, 2025

With the InformativeNotification, if NewTools is in the image, we ask it to inform the user via growl (in the morphic adapter).

But if the image is not interactive, then we do nothing while we should print the string in the stdout!

yes, you're right @jecisc !
Tests are almost ok but there is a pb with MCStWriter tests: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-18906/lastCompletedBuild/testReport/

@demarey
Copy link
Contributor

demarey commented Nov 19, 2025

I will not fix what I do not understand. Sorry but I cannot. I do not even know the code and this is probably bash code. @demarey may be you know. For me I will merge the PR since Pharo tests are green.

These tests are important @Ducasse : they ensure Pharo command line is working as expected.
As explained by @jecisc , they are written in bash (using a test lib) because we are black box testing these components.
They are located in https://github.com/pharo-project/pharo/tree/Pharo14/tests and are quite easy to read.
Here is an example:

@test "test can run tests on a regex" {
  run_pharo test SUnit-UI.*
  assert_success
  assert_line "Running tests in 2 Packages"
  assert_line "Finished running 16 Tests"
  assert_line --partial "Finished to run tests of SUnit-UI-Tests in"
  assert_line "16 run, 16 passes, 0 failures, 0 errors."
}

Tests use helper functions defined in https://github.com/pharo-project/pharo/blob/Pharo14/tests/test_helper.bash.
This way, tests are really easy to read: we run a pharo command, we assert exit code, and some expected output

GitHub
Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk. - pharo-project/pharo
GitHub
Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk. - pharo-project/pharo

@Ducasse
Copy link
Member Author

Ducasse commented Nov 19, 2025

Of course they are important now I do not know how they fail.
So is my revert ok?

@Ducasse
Copy link
Member Author

Ducasse commented Nov 19, 2025

So yes my revert fixed the problems with clap. Now this is strange because before the MC testswere not failing.

@demarey
Copy link
Contributor

demarey commented Nov 19, 2025

So yes my revert fixed the problems with clap. Now this is strange because before the MC testswere not failing.

yes, The problem is the one @jecisc pointed out. In headless mode, no inform is performed on stdout as expected

@jecisc jecisc merged commit faf6a1d into pharo-project:Pharo14 Nov 19, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants