Test with Julienne and Gfortran 15 #251
Conversation
bonachea
left a comment
bonachea
left a comment
There was a problem hiding this comment.
@rouson it appears you've inadvertently committed three copies of the GASNet install tree to this PR branch, which makes it very difficult to review the PR and probably at least partially explains the CI failures. These stray directories must be deleted from the branch and squashed out of the git history, to ensure these binaries don't eventually end up merged and bloat the Caffeine git repo.
Would you like to address that chore or shall I?
|
@bonachea Oops! Sorry. Please fix this when you have time -- no rush. |
|
I removed the GASNet install trees, but now I notice other stray files that appear to have also been inadvertently added. I'll assume I also have your permission to remove those. |
|
@bonachea I fixed the issue that was causing failures. All tests pass now. This PR is ready for review. |
|
As discussed in our meeting today, We can always rename files later in a subsequent PR that omits content changes, assuming there's a compelling motivation for it. CI tests are still passing, and I hope to find time to review this PR next week. |
|
@bonachea sounds good. I really like Caffeine's coding conventions and hadn't noticed them before. |
bonachea
left a comment
bonachea
left a comment
There was a problem hiding this comment.
Overall we are in agreement to move towards Julienne-based testing, ideally completing that transition in the next five weeks.
My main objection to this PR as it currently stands is the introduction of code duplication for the HAVE_PROCEDURE_ACTUAL_FOR_POINTER_DUMMY workaround.
As discussed in our Caffeine meeting today, as the next step for this PR I'd like us to review and merge the Julienne PR for the bless workaround and create a new Julienne release providing that support, then update this PR to import that new Julienne release. That will enable us to excise all the HAVE_PROCEDURE_ACTUAL_FOR_POINTER_DUMMY code duplication currently in this PR, replacing it with the far more concise and maintainable workaround.
|
@bonachea is this PR ready to merge? GitHub indicates that there is one change requested but when I click the link to see the requested change, I get the message "We went looking everywhere, but couldn’t find those commits." |
bonachea
left a comment
bonachea
left a comment
There was a problem hiding this comment.
Hi @rouson - Apologies this PR has dragged on for so long. As you know, there's been other pressing priorities on our time. However I think we agree we're now ready to proceed with the Julienne conversion (of which this PR is the first step).
I think this PR is very close to ready. However it has also changed considerably since you first wrote it; I did alot of work to address changes I considered important but hopefully non-controversial. This notably includes a number of cosmetic/stylistic updates I pushed last night. As such, I'd like to ensure you have also reviewed the PR in its current form.
I've also added a number of comments/suggestions for areas where I'd like your explicit attention/input.
| diag = & | ||
| .all. (real(my_val, c_double) .approximates. real(expected, c_double) .within. tolerance) & | ||
| .also. (.all. (real(aimag(my_val), c_double) .approximates. real(aimag(expected), c_double) .within. tolerance)) |
There was a problem hiding this comment.
@rouson It would be nice if Julienne's .approximates. had direct support for complex types, so that the user doesn't have to write awkward expressions like the one above; I'd rather be able to write simply:
my_val .approximates. expected .within. tolerance
where my_val and expected have type complex(rkind) and tolerance has type real(rkind).
The current code above presumably also generates a less helpful error message when only one of the components is outside tolerance.
This suggestion is mostly orthogonal to this PR and not a condition for merge, just an observation and suggestion for improvement to Julienne. Perhaps if you agree, then open an RFE issue on Julienne?
| diag = .expect. (any(image_status == [0, PRIF_STAT_FAILED_IMAGE, PRIF_STAT_STOPPED_IMAGE])) & ! TODO: replace with .any. once Juliennes supports it | ||
| // "permitted image status" |
There was a problem hiding this comment.
IIUC, a failure of this test would not print out the failing image_status value, only a false .expect. (analogously to Veggies).
I'm not very worried about this particular case, but I could imagine other situations arising where a result value is permitted to have one of several distinct expected values, and it would be nice for the diagnostic to include the actual value when that comparison fails to match any of the expected values.
Is there an open feature request to add an .any. operator in Julienne to improve this diagnostic behavior?
This commit adds a test-suite driver, test/julienne/driver.f90, with supporting tests for the following subroutines: - prif_co_broadcast - prif_co_min - prif_co_max - prif_image_queries - prif_init - prif_num_images - prif_this_image_no_coarray - prif_sync_images All new tests are in test/julienne. All tests pass. TODO: restrict output to image 1 chore: rm redundant tests This commit removes veggies tests for the following subroutines because they are now redundant with the correspondiong julienne tests added in a prior commit: - prif_co_broadcast - prif_co_max - prif_co_min - prif_image_queries - prif_num_images - prif_sync_images - prif_this_image This commit retains the redundant prif_init test becuase it is presumalby needed for the proper launch of the veggies tests. build(fpm.toml.template): add Julienne 3.0.0 dep test(co_sum): add julienne test, rm veggies test test(co_reduce): add julienne test|rm veggies test chore: non_overridable test_t child type-bnd-procs fix(image_queries_test): add closing parens test: add prif_coarray_inquiry_test_m fix: rm binary chore: rm veggies prif_coarray_inquiry_test fix(test/julienne): support GCC 13 - 14.2 chore: rm reference to deleted veggies test chore: rm partially complete julienne test build(include): fix macro logic/syntax test(julienne): append to diagnostics strings This commit appends the text from veggies assertions "message" argument to test diagnoses in the corresponding Julienne tests. For example, a veggies assertion of the form assert_equals(actual, expected, message) becomes a Julienne test diagnosis of the following form: (actual .equalsExpected. expected) // message Remove inadvertently added GASNet install trees Remove some stray programs Rename file to match Caffiene source file naming conventions (See docs/README-maintainers.md) test(julienne): fix GCC 13-14.2 workaround Rename test files back to their original location, for ease of PR review and maintenance There are zero code changes in this commit doc(julienne): switch to Caffeine's copyright Co-authored-by: Dan Bonachea <dobonachea@lbl.gov>
This header is no longer relevant to the test code
Minimize extraneous verbiage in subject() strings to make the output easier to visually skim None of the PRIF procedures are functions, and the function/subroutine distinction is irrelevant in test output
This abbreviation helps with readability, especially for the incremental test idiom (ie. `test_diagnosis = test_diagnosis .also. `). Restore line break conventions used in the incremental test idiom, which emphasizes readability of the relevant expression. Replace `.expect. .true.` with the shorter idiom in newer Julienne
Prefer 64-bit comparison for any use case where a truncating conversion could plausibly mask a defect. Also, use native kind real comparison whenever possible.
Reorder unit-testing to roughly start from the simplest and most fundamental features, as these are assumed to work correctly by later tests of more complicated features.
Co-authored-by: Dan Bonachea <dobonachea@lbl.gov>
…tion Julienne guarantees that `test_fixture_t` elements in a `test_harness_t` are executed strictly in-order, so we don't need a hack to ensure proper initialization ordering.
|
Rebasing to pick-up conflicting CI changes in main |
2 participants
This PR
test/juliennethat replace corresponding veggies tests intest/.test/julienne/driver.F90.The decision about which tests to prioritize for transitioning first was driven by file size with the work progressing from the smallest file to the largest file. A couple of relatively minor features have been added in Julienne 3.1.0 in order to support Caffeine, but there no known issues preventing continuing this process with the remaining tests. At this point, the only known hurdle is available developer time.