[FIX] issue 1637 #1720

t-h2o · 2024-11-17T20:59:59Z

Fix the dry run flag

issue 1637

TODO

fix the bug
- --dry-run flag set variable
- dryRun avoid to read config
rename dry run by no config
write a better flag description to no config
Also add a test for it in application/testing/CMakeLists.txt
- screenshot with no-config

application/F3DStarter.cxx

application/testing/CMakeLists.txt

mwestphal · 2024-11-24T06:34:05Z

Do you need help moving forward @t-h2o ?

t-h2o · 2024-11-25T00:26:29Z

Do you need help moving forward @t-h2o ?

yes, I am not familiar with F3D testing framework.

My goal is to test that no-config is set to 1:

'no-config' = '1' from CLI options

For that, I run this command

f3d --no-render --config=config_build --no-config --verbose

In application/testing/CMakeLists.txt

NOTE --no-render and --verbose are already set in the f3d test function -> I remove them

My result for lines to add into application/testing/CMakeLists.txt is... :

# Test that --no-config set no-config value to 1
f3d_test(NAME TestNoConfigWithConfig ARGS --no-config --config=config_build --verbose REGEXP "'no-config' = '1'" NO_BASELINE)

f3d-app#1720 (comment)

mwestphal · 2024-12-05T22:16:29Z

Please let me know if you need another review @t-h2o :)

t-h2o · 2024-12-10T10:27:06Z

Please let me know if you need another review @t-h2o :)

Hi @mwestphal, I have these two unanswered comments:

mwestphal · 2024-12-10T10:38:00Z

Do you need help moving forward @t-h2o ?

yes, I am not familiar with F3D testing framework.

My goal is to test that no-config is set to 1:
'no-config' = '1' from CLI options
For that, I run this command
f3d --no-render --config=config_build --no-config --verbose
In application/testing/CMakeLists.txt

NOTE --no-render and --verbose are already set in the f3d test function -> I remove them

My result for lines to add into application/testing/CMakeLists.txt is... :
# Test that --no-config set no-config value to 1
f3d_test(NAME TestNoConfigWithConfig ARGS --no-config --config=config_build --verbose REGEXP "'no-config' = '1'" NO_BASELINE)

This looks correct to me

mwestphal · 2024-12-10T10:38:31Z

Please let me know if you need another review @t-h2o :)

Hi @mwestphal, I have these two unanswered comments:
* [[FIX] issue 1637 #1720 (comment)](https://github.com/f3d-app/f3d/pull/1720#issuecomment-2496450147)

* [[FIX] issue 1637 #1720 (comment)](https://github.com/f3d-app/f3d/pull/1720#discussion_r1855644894)

I have answered your questions :)

mwestphal · 2024-12-16T20:01:23Z

Do you need help moving forward @t-h2o ?

f3d-app#1720 (comment)

application/testing/CMakeLists.txt

mwestphal

missing TestNoConfigWithConfig

application/testing/CMakeLists.txt

mwestphal · 2024-12-17T19:29:12Z

Looks like a few console test are failing, can you check them locally ?

mwestphal · 2024-12-18T16:06:00Z

Looks like a few console test are failing, can you check them locally ?

You actually just need to update the baselines :)

f3d-app#1720 (comment)

t-h2o · 2024-12-18T20:57:29Z

You actually just need to update the baselines :)

What do you mean by update the baselines?
Rebase on the origin/master?

mwestphal · 2024-12-18T21:44:10Z

I mean that your change has an impact on some test that had an incorrect baseline because of the dry run issue. You can run these test locally and recover the baseline

mwestphal · 2024-12-19T07:04:17Z

You can look in the CI results to see which test are impacted.

mwestphal · 2024-12-20T17:36:07Z

Hi @t-h2o

We will need this before January 3 to start the release cycle. So you want me to take over the PR or do you want to finish it ?

t-h2o · 2024-12-21T19:41:15Z

So you want me to take over the PR or do you want to finish it ?

No/Yes, my next update will be before the Mon Dec 23 11:00:00 PM CET 2024.

mwestphal · 2024-12-21T21:13:23Z

So you want me to take over the PR or do you want to finish it ?

No/Yes, my next update will be before the Mon Dec 23 11:00:00 PM CET 2024.

Thanks for the feedback! Let me know if you need any help.

mwestphal · 2024-12-23T09:14:46Z

You now need to rebase on latest master, many changes have been merged.

mwestphal · 2024-12-25T14:05:32Z

I updated other image with the function, for each failed test, I ran the function

$ function f3d-update-image() {
  ctest -R "${1}"
  mv Testing/Temporary/"${1}".png testing/baselines/"${1}".png
  git add -p <<< y
  git commit -m "test: update ${1}"
}
$ f3d-update-image TestInteractionConsoleOpenWarningKeyboard

Indeed, all new baselines are correct!

--dry-run flag set variable dryRun avoid to read config

in case dry-run or no-render is true, we do not read the configuration file from f3d --help: [...] --dry-run [=<bool>(=1)] Do not read the configuration file (default: false) --no-render [=<bool>(=1)] Do not read the configuration file (default: false) [...]

find . -name "*\.cxx" -exec sed -i 's/dry-run/no-config/g' {} \;

find . -name "*\.cxx" -exec sed -i 's/dryRun/noConfig/g' {} \;

find . -name "*\.txt" -exec sed -i 's/--dry-run/--no-config/g' {} \;

find . -name "*\.h" -exec sed -i 's/dry-run/no-config/g' {} \; find . -name "*\.md" -exec sed -i 's/dry-run/no-config/g' {} \; find . -name "*\.txt" -exec sed -i 's/DryRun/NoRender/g' {} \;

f3d-app#1720 (comment)

test it with ctest -R NoConfigWithConfig

bin/f3d \ --output testing/baselines/TestNoConfigWithConfig.png \ --no-config \ --resolution=300,300 \ --config testing/configs/complex.json \ testing/data/f3d.glb

application/testing/CMakeLists.txt

mwestphal

one last small change :)

Co-authored-by: Mathieu Westphal <[email protected]>

t-h2o · 2024-12-26T12:05:49Z

Yet, we should be good :)

mwestphal · 2024-12-26T13:39:42Z

Thanks for your contribution @t-h2o !

t-h2o · 2024-12-26T14:53:37Z

Nice to participate in the open source world!
I learned a lot from this contribution.
Thanks for the review.

t-h2o mentioned this pull request Nov 17, 2024

--dry-run option is not working #1637

Closed

mwestphal reviewed Nov 17, 2024

View reviewed changes

application/F3DStarter.cxx Outdated Show resolved Hide resolved

mwestphal reviewed Nov 18, 2024

View reviewed changes

application/F3DStarter.cxx Show resolved Hide resolved

mwestphal reviewed Nov 19, 2024

View reviewed changes

application/testing/CMakeLists.txt Outdated Show resolved Hide resolved

t-h2o added a commit to t-h2o/f3d that referenced this pull request Nov 25, 2024

test: rename NoNoRender -> NoNoConfig

dbbf44b

f3d-app#1720 (comment)

t-h2o added a commit to t-h2o/f3d that referenced this pull request Dec 17, 2024

test: rename NoNoRender -> NoNoConfig

1ca8045

f3d-app#1720 (comment)

t-h2o force-pushed the issue-1637 branch from dbbf44b to 1ca8045 Compare December 17, 2024 10:47

mwestphal approved these changes Dec 17, 2024

View reviewed changes

mwestphal reviewed Dec 17, 2024

View reviewed changes

application/testing/CMakeLists.txt Show resolved Hide resolved

mwestphal requested changes Dec 17, 2024

View reviewed changes

t-h2o marked this pull request as ready for review December 17, 2024 11:26

mwestphal reviewed Dec 17, 2024

View reviewed changes

application/testing/CMakeLists.txt Outdated Show resolved Hide resolved

t-h2o added a commit to t-h2o/f3d that referenced this pull request Dec 18, 2024

test: rename NoNoRender -> NoNoConfig

fdf329b

f3d-app#1720 (comment)

t-h2o force-pushed the issue-1637 branch from 2958d3c to 189f1b3 Compare December 18, 2024 20:33

t-h2o added 18 commits December 25, 2024 23:50

[FIX] issue 1637

41c55ce

--dry-run flag set variable dryRun avoid to read config

OptionsTools: change no-render description

54f6e9c

Starter: remove no-render

a227436

rename dry-run by no-config

da87c4d

find . -name "*\.cxx" -exec sed -i 's/dry-run/no-config/g' {} \;

rename dryRun by noConfig

22e9500

find . -name "*\.cxx" -exec sed -i 's/dryRun/noConfig/g' {} \;

rename --dry-run by --no-config

76cc630

find . -name "*\.txt" -exec sed -i 's/--dry-run/--no-config/g' {} \;

rename dry-run by no-config

b220be6

find . -name "*\.h" -exec sed -i 's/dry-run/no-config/g' {} \; find . -name "*\.md" -exec sed -i 's/dry-run/no-config/g' {} \; find . -name "*\.txt" -exec sed -i 's/DryRun/NoRender/g' {} \;

test: rename NoNoRender -> NoNoConfig

a75598e

f3d-app#1720 (comment)

test: add NoConfigWithConfig

283f547

test it with ctest -R NoConfigWithConfig

test: update TestInteractionConsoleInvalidCommand

74d875a

test: update TestInteractionConsoleInvalidOption

f8f57cd

test: update TestInteractionConsoleTypingSceneInfo

0e0e2b6

test: update TestInteractionConsoleOpen

dfd369b

test: update TestInteractionConsoleOpenWarningKeyboard

f4e9bef

test: update TestInteractionConsoleOpenWarningClick

b16c8c0

test: TestNoConfigWithConfig: create image

e31e88b

bin/f3d \ --output testing/baselines/TestNoConfigWithConfig.png \ --no-config \ --resolution=300,300 \ --config testing/configs/complex.json \ testing/data/f3d.glb

test: TestNoConfigWithConfig: replace verbose

6e6b07c

t-h2o force-pushed the issue-1637 branch from 72cf7dd to 6e6b07c Compare December 25, 2024 22:54

mwestphal reviewed Dec 26, 2024

View reviewed changes

application/testing/CMakeLists.txt Outdated Show resolved Hide resolved

mwestphal requested changes Dec 26, 2024

View reviewed changes

test: TestNoConfigWithConfig: clearer comment

c2eb8f5

Co-authored-by: Mathieu Westphal <[email protected]>

mwestphal approved these changes Dec 26, 2024

View reviewed changes

mwestphal merged commit 7a649d4 into f3d-app:master Dec 26, 2024
43 checks passed

t-h2o deleted the issue-1637 branch December 26, 2024 14:48

mwestphal mentioned this pull request Dec 26, 2024

Renamed dry-run argument to no-config #1690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] issue 1637 #1720

[FIX] issue 1637 #1720

t-h2o commented Nov 17, 2024 •

edited

Loading

mwestphal commented Nov 24, 2024

t-h2o commented Nov 25, 2024

mwestphal commented Dec 5, 2024

t-h2o commented Dec 10, 2024

mwestphal commented Dec 10, 2024

mwestphal commented Dec 10, 2024

mwestphal commented Dec 16, 2024

mwestphal left a comment

mwestphal commented Dec 17, 2024

mwestphal commented Dec 18, 2024

t-h2o commented Dec 18, 2024

mwestphal commented Dec 18, 2024

mwestphal commented Dec 19, 2024

mwestphal commented Dec 20, 2024

t-h2o commented Dec 21, 2024

mwestphal commented Dec 21, 2024

mwestphal commented Dec 23, 2024

mwestphal commented Dec 25, 2024

mwestphal left a comment

t-h2o commented Dec 26, 2024

mwestphal commented Dec 26, 2024

t-h2o commented Dec 26, 2024

[FIX] issue 1637 #1720

[FIX] issue 1637 #1720

Conversation

t-h2o commented Nov 17, 2024 • edited Loading

Fix the dry run flag

TODO

mwestphal commented Nov 24, 2024

t-h2o commented Nov 25, 2024

mwestphal commented Dec 5, 2024

t-h2o commented Dec 10, 2024

mwestphal commented Dec 10, 2024

mwestphal commented Dec 10, 2024

mwestphal commented Dec 16, 2024

mwestphal left a comment

Choose a reason for hiding this comment

mwestphal commented Dec 17, 2024

mwestphal commented Dec 18, 2024

t-h2o commented Dec 18, 2024

mwestphal commented Dec 18, 2024

mwestphal commented Dec 19, 2024

mwestphal commented Dec 20, 2024

t-h2o commented Dec 21, 2024

mwestphal commented Dec 21, 2024

mwestphal commented Dec 23, 2024

mwestphal commented Dec 25, 2024

mwestphal left a comment

Choose a reason for hiding this comment

t-h2o commented Dec 26, 2024

mwestphal commented Dec 26, 2024

t-h2o commented Dec 26, 2024

t-h2o commented Nov 17, 2024 •

edited

Loading