Skip to content

Conversation

@Yushan-Wang
Copy link
Member

@Yushan-Wang Yushan-Wang commented Nov 18, 2025

While doing other stuff, I noticed this "typo".
However, it doesn't result in any errors.
I don't think it's worth filling in the checks below.
Only the copyright date is updated.

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@Yushan-Wang Yushan-Wang marked this pull request as ready for review November 18, 2025 12:04
if [[ "01234567" == *"${JOBID}"* ]]; then export PDI_PLUGIN_PATH=/tmp/pdi_plugins; fi
export MAKEFLAGS='-j 4'
export CTEST_FLAGS="--output-junit /tmp/tests.xml"
export TEST_DIR="/tmp_dir_test"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is related to the CI, which breaks regularly.
Does this need a dedicated PR?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily for me, but is this directory always available. Shouldn't you then mount a tmpdir to ensure it's available?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily for me, but is this directory always available. Shouldn't you then mount a tmpdir to ensure it's available?

@JAuriac

Copy link
Member

Choose a reason for hiding this comment

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

I would not include this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This TEST_DIR is necessary for another ongoing PR, which includes a fix.
#543 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. We will wait for the no-pdi PR first.
Anyway, if I exclude this change, the CI will not run, thus the PR can not be approved.

Copy link
Member

Choose a reason for hiding this comment

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

So, what's the status, should this change remain in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change will be overwritten by the no-pdi PR, where we use export CTEST_DIR="/tmp", so this change does not have to remain in this PR.

jbigot
jbigot previously approved these changes Nov 18, 2025
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

good catch!

if [[ "01234567" == *"${JOBID}"* ]]; then export PDI_PLUGIN_PATH=/tmp/pdi_plugins; fi
export MAKEFLAGS='-j 4'
export CTEST_FLAGS="--output-junit /tmp/tests.xml"
export TEST_DIR="/tmp_dir_test"
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily for me, but is this directory always available. Shouldn't you then mount a tmpdir to ensure it's available?

@jbigot
Copy link
Member

jbigot commented Nov 18, 2025

can we make a test that would catch this bug?

@Yushan-Wang
Copy link
Member Author

can we make a test that would catch this bug?

I can try!

@Yushan-Wang Yushan-Wang requested a review from jbigot November 19, 2025 12:57
JAuriac
JAuriac previously approved these changes Nov 25, 2025
jbigot
jbigot previously approved these changes Nov 25, 2025
Copy link
Member

@jbigot jbigot 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 the test logic is slightly above my level of understanding. I would love for you to guide me through it, and mybe add a few comments, so that we're not lost the next time we need to touch this.

if [[ "01234567" == *"${JOBID}"* ]]; then export PDI_PLUGIN_PATH=/tmp/pdi_plugins; fi
export MAKEFLAGS='-j 4'
export CTEST_FLAGS="--output-junit /tmp/tests.xml"
export TEST_DIR="/tmp_dir_test"
Copy link
Member

Choose a reason for hiding this comment

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

So, what's the status, should this change remain in this PR?

@Yushan-Wang Yushan-Wang dismissed stale reviews from jbigot and JAuriac via c6f1da8 November 26, 2025 12:54
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Have you checked that the current writing of the test does indeed catch the bug if you don't fix the code?

@jbigot
Copy link
Member

jbigot commented Nov 26, 2025

BTW, this is a bugfix and should be mentioned in the Changelog

@Yushan-Wang
Copy link
Member Author

Have you checked that the current writing of the test does indeed catch the bug if you don't fix the code?

Yes, it does.

@Yushan-Wang Yushan-Wang requested a review from jbigot November 28, 2025 11: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.

4 participants