Skip to content

Fix split-vars tests#355

Closed
ceblanton wants to merge 2 commits intomainfrom
fix-splitvars-tests
Closed

Fix split-vars tests#355
ceblanton wants to merge 2 commits intomainfrom
fix-splitvars-tests

Conversation

@ceblanton
Copy link
Copy Markdown
Contributor

Description
The new split-ncvars tests cannot find the just-built script and executable location.

This resolves that problem and does not add any new functionality.

Fixes #353

How Has This Been Tested?
Hand-tested, and CI tests will pass

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

@ceblanton ceblanton requested a review from underwoo March 17, 2025 19:58
@ceblanton
Copy link
Copy Markdown
Contributor Author

@underwoo , @mlee03 I'm sorry for merging #354 despite its failing tests. This PR fixes the tests.

@mlee03
Copy link
Copy Markdown
Contributor

mlee03 commented Mar 18, 2025

@underwoo, a quick review please :)

Copy link
Copy Markdown
Member

@underwoo underwoo left a comment

Choose a reason for hiding this comment

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

I cannot approve this PR until the items identified are fixed.

I suggest working with PR #338, and adding any adjustments off that PR. We need to get that PR approved and in place.

. ${srcdir=.}/init.sh; path_prepend_ ../src

# Set paths to list-vars script and just-compiled exec
export LIST_NCVARS=`find ../.. -name list_ncvars.sh`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should be added to the Makefile.am where the test environment is defined, not here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is true for all other files.

PR #338 has a fix for the PKGLIBEXECDIR. Please work to have that merged as well.

? $ENV{LIST_NCVARS}
: "$prefix/bin/list_ncvars.sh";

unless (-f $list_ncvars) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A test scripot is required to ensure this line does what you expect.

Also, only testing if the file exists is not sufficient. You do need to verify the command runs. It would be easier to verify that the command ran successfully in the location where the command is run in this script than just trying to figure out if the command exits and is executable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much. I that checking the exit status would be better than this check, and I'll do that in a separate PR.

@ceblanton
Copy link
Copy Markdown
Contributor Author

#338 resolves the split-vars test failures in a far better way, so closing this with prejudice.

@ceblanton ceblanton closed this Mar 18, 2025
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.

split_ncvars.pl has incorrect path for list_ncvars

3 participants