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

Replace parallel statements with Caffeine calls #136

Merged
merged 15 commits into from
Sep 12, 2024
Merged

Conversation

rouson
Copy link
Collaborator

@rouson rouson commented Sep 12, 2024

This PR

  1. Replaces several occurrences of error stop and sync all with the prif_error_stop and prof_sync_all calls, respectively.
  2. Adds a test for prif_error_stop with no stop code.
  3. Defines a unit_test_parameters_m module with constants for use in different parts of the test suite.

This replaces the `sync all` statement in program_termination_s
with call prif_sync_all so that we use the prif/caffeine
capability rather than mistakenly invoking a compiler's
pre-existing parallel statement support if it exists.
This replaces the `error stop` statement in example/hello.f90
with `call prif_error_stop` so that we use the prif/caffeine
capability rather than mistakenly invoking a compiler's
pre-existing parallel statement support if it exists.
This commit
1. Replaces all occurrences of `error stop` with
   `call prif_error_stop` in the programs launched by unit tests
   for program termination and
2. Expands the number of such programs so that one each program
   each tests the six cases of normal and error termination with
   integer stop code, character stop code, and no stop code.
3. Adds unit_test_parameters_m to put expected stop codes in one
   place for use by unit tests, including the programs in
   example/support-test that support unit tess in test/.
Copy link
Collaborator

@ktras ktras left a comment

Choose a reason for hiding this comment

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

Looks good but needs a couple simple changes.

example/support-test/error_stop_with_no_code.f90 Outdated Show resolved Hide resolved
src/caffeine/unit_test_parameters_m.mod Outdated Show resolved Hide resolved
test/caf_error_stop_test.f90 Outdated Show resolved Hide resolved
@ktras ktras self-requested a review September 12, 2024 00:45
test/.gitignore Outdated Show resolved Hide resolved
test/caf_error_stop_test.f90 Outdated Show resolved Hide resolved
This commit fixes some of the example/support-test program names
to match the base file names and adds cmdstat and cmd_msg arguments
to the execute_command_line invocations in the test suite.  The
latter change should facilitate capturing richer information about
error conditions resulting form launching a child process to run
one of the programs employed for testing normal and error
termination.
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

Noted a few minor but important changes I'd like to see.

Not represented in my review is the fact I consider the entire approach taken in the exit testing of invoking fpm from within a unit test and inspecting the exit code to be very fragile. I've just entered this as issue #137

example/support-test/README.md Outdated Show resolved Hide resolved
example/support-test/error_stop_with_character_code.f90 Outdated Show resolved Hide resolved
example/support-test/stop_with_character_code.f90 Outdated Show resolved Hide resolved
example/hello.f90 Outdated Show resolved Hide resolved
rouson and others added 4 commits September 12, 2024 10:07
Co-authored-by: Dan Bonachea <dobonachea@lbl.gov>
This will allow a human to audit that the string appears in the output when run manually.

Co-authored-by: Dan Bonachea <dobonachea@lbl.gov>
This will allow a human to audit that the provided string appears in the output.

Co-authored-by: Dan Bonachea <dobonachea@lbl.gov>
@ktras ktras requested review from ktras and bonachea September 12, 2024 17:19
Copy link
Collaborator

@ktras ktras left a comment

Choose a reason for hiding this comment

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

LGTM with the understanding that after the upcoming release, we will work to improve the error stop and stop tests documented in issue #137.

Copy link
Member

@bonachea bonachea 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 merging the changes. IMO overall this PR is definitely an improvement, and I'd like to see it in the release.

I've identified one other serious defect in the exit tests modified by this PR, which are not working as intended even in shared memory. The same defect also exists in main, so this is not technically a regression.

Given the impending release and the fundamental problems documented in #137 that make this entire approach to exit-testing a non-starter in general, fixing this particular defect is basically "putting lipstick on a pig". So I'm also fine ignoring the problem for now and merging as-is.

test/caf_error_stop_test.f90 Show resolved Hide resolved
test/main.f90 Outdated Show resolved Hide resolved
Co-authored-by: Dan Bonachea <dobonachea@lbl.gov>
@ktras ktras merged commit c4791e2 into main Sep 12, 2024
6 checks passed
@ktras ktras deleted the call-prif_sync_all branch September 12, 2024 21:03
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