Fix path resolver and add debugging output #21

andy5995 · 2024-11-28T06:56:52Z

I don't know much about your preferences or how you like to configure tests, but I took a guess and made a few quick changes.

With the changes in this PR, when meson test is run with -v, one can see more:

stderr:
╭<parent> /hello world/banana/
├<child>  quick-js/././ww/aee/
├>        /hello world/banana/quick-js/ww/aee
╰>>       /hello world/banana/quick-js/ww/aee/
╭<parent> /hello world/banana/
├<child>  quick-js/././ww/aee/
├>        /hello world/banana/quick-js/ww/
╰>>       /hello world/banana/quick-js/ww/aee/
╭<parent> /hello world/banana/
├<child>  quick-js/././ww/aee
├>        /hello world/banana/quick-js/ww/
╰>>       /hello world/banana/quick-js/ww/
╭<parent> /hello world/banana/
├<child>  ../../quick-js/ww/a
├>        /quick-js/ww/a
╰>>       /quick-js/ww/a
╭<parent> /hello world/banana/
├<child>  ./../.././quick-js/ww/a
├>        /quick-js/ww/a
╰>>       /quick-js/ww/a
╭<parent> /hello world/banana/
├<child>  ../../quick-js/ww/a
├>
╰>>
╭<parent> /hello world/banana/
├<child>  ./../.././quick-js/ww/a
├>
╰>>
╭<parent> /hello world/banana/
├<child>  quick-js/../quick-js/ww/a
├>        /hello world/banana/quick-js/ww/a
╰>>       /hello world/banana/ww/a
╭<parent> /hello world/banana/
├<child>  quick-js/./.././ww/a
├>        /hello world/banana/ww/a
╰>>       /hello world/banana/ww/a
╭<parent> /a/
├<child>  b../
├>        /a/
╰>>       /a/b../
ret: 4
test_resolver-paths: ../test/marginal/resolver-paths.cpp:33: int main():
Assertion `ret=0' failed.

andy5995 · 2024-11-28T07:09:40Z

Oh, yes, it would be nice if I fixed the actual problem, and I'd be happy to do that.

:) I did look at the function a few days ago but I couldn't figure out how it worked and haven't gone back to it yet.

andy5995 · 2024-11-28T07:16:43Z

I don't know much about your preferences or how you like to configure tests

I looked at #10 (comment) again and see your intention is to apparently output stdout to a log file.

Feel free to close this PR then. ;)

KaruroChori · 2024-11-28T13:06:57Z

I don't know much about your preferences or how you like to configure tests, but I took a guess and made a few quick changes.

Usually I let it run without output as part of the suite.
When I see the test fails, I manually run it from ./build/ so that its output is visible and I can down the error.

I don't think I am in favour or using cerr in this instance because of its semantics.
If my test code was to open a resource file to operate, like a list of actions to perform on the UI encoded as XML, and fail, that would be something to report on console error.
But normal output which is part of its expected execution should not.

Oh, yes, it would be nice if I fixed the actual problem, and I'd be happy to do that.

That function is totally broken :D. To be fair, it was quickly developed at a stage in which I had defined no formal specifications on its behaviour.
This test file was actually me writing down more proper expectations on how it should operate so that I have a reference when I do it again.

For reference, this is what the high level description of it would say if documented :D

It glues the base path and the child path normalizing the child (reducing /../ and /./).
The first boolean flag is true if it can eat tokens from the parent while performing (/../).
The second flag if true should remove the filename if child is the path to a file (not / terminated), or ignore this functionality if a directory (/ terminated).

andy5995 · 2024-11-30T00:18:47Z

Usually I let it run without output as part of the suite. When I see the test fails, I manually run it from ./build/ so that its output is visible and I can down the error.

I don't think I am in favour or using cerr in this instance because of its semantics. If my test code was to open a resource file to operate, like a list of actions to perform on the UI encoded as XML, and fail, that would be something to report on console error. But normal output which is part of its expected execution should not.

Thanks for explaining. Sadly I don't understand the case you're talking about yet, lol

If there's some way using cerr or fprintf(stderr, ...) in some tests could be done, while not screwing up something else, that would be good.

If it helps at all, Adding -v isn't strictly necessary. stderr will always go to build/meson-logs/testlog.txt regardless of whether -v is used.

And in the CI, you can view that log:

And one can view output on the console from a single test by using meson test test_resolver-paths -v

However, I don't see any need to make a final decision. Better to keep doing the good job you're doing now working on improving the tests. :)

andy5995 · 2024-12-05T05:13:48Z

test/marginal/resolver-paths.cpp

@@ -30,6 +30,7 @@ int main(){
    ret+=test_normalizer("/hello world/banana/ww/a", "/hello world/banana/", "quick-js/./.././ww/a", false, false);
    ret+=test_normalizer("/a/b../", "/a/", "b../", false, false);

-    assert(ret=0);
+    std::cerr<<"ret = "<<ret<<"\n";
+    assert(ret==0);


This fixes where the test fails even when ret is eq to 0 ;)

LOL in theory my IDE should have warned me, somehow I missed its message.

andy5995 · 2024-12-05T05:15:03Z

test/marginal/resolver-paths.cpp

-    std::cout<<"├<child>  "<<child<<"\n";
-    std::cout<<"├>        "<<tmp.second<<"\n";
-    std::cout<<"╰>>       "<<expected<<"\n";
+    std::cerr<<"╭<parent> "<<parent<<"\n";


I know we discussed this change. If I manage to fix the resolver, I'll revert these changes before merging.

andy5995 · 2024-12-05T05:22:15Z

2/4 failures are fixed with this:

╭<parent> /hello world/banana/
├<child>  quick-js/././ww/aee/
├>        /hello world/banana/quick-js/ww/aee/
╰>>       /hello world/banana/quick-js/ww/aee/
╭<parent> /hello world/banana/
├<child>  quick-js/././ww/aee/
├>        /hello world/banana/quick-js/ww/aee/
╰>>       /hello world/banana/quick-js/ww/aee/

Although I can't say with certainty that it will always give you the intended result. The conditions happens whether 'base_dir' is true or false.

The other two failures:

╭<parent> /hello world/banana/
├<child>  quick-js/../quick-js/ww/a
├>        /hello world/banana/quick-js/ww/a
╰>>       /hello world/banana/ww/a

╭<parent> /a/
├<child>  b../
├>        /a/
╰>>       /a/b../

I don't understand why the expected results are how you have them. I've never known a valid filename or directory to contain '..' unless the dots are by themselves. And the first failure there... I don't understand why quickjs isn't part of the expected path.

KaruroChori · 2024-12-05T05:36:30Z

I don't understand why the expected results are how you have them. I've never known a valid filename or directory to contain '..' unless the dots are by themselves. And the first failure there... I don't understand why quickjs isn't part of the expected path.

Because the test is wrong. The expected result should have been /hello world/banana/quick-js/ww/a as well.
As for files, any arbitrary number of dots is legal in many filesystems (excluding special cases . and ..) .
Just try touching a file with such name :D.

I don't know much about your preferences or how you like to configure tests, but I took a guess and made a few quick changes. With the changes in this PR, when `meson test` is run with `-v`, one can see more: ``` stderr: ╭<parent> /hello world/banana/ ├<child> quick-js/././ww/aee/ ├> /hello world/banana/quick-js/ww/aee ╰>> /hello world/banana/quick-js/ww/aee/ ╭<parent> /hello world/banana/ ├<child> quick-js/././ww/aee/ ├> /hello world/banana/quick-js/ww/ ╰>> /hello world/banana/quick-js/ww/aee/ ╭<parent> /hello world/banana/ ├<child> quick-js/././ww/aee ├> /hello world/banana/quick-js/ww/ ╰>> /hello world/banana/quick-js/ww/ ╭<parent> /hello world/banana/ ├<child> ../../quick-js/ww/a ├> /quick-js/ww/a ╰>> /quick-js/ww/a ╭<parent> /hello world/banana/ ├<child> ./../.././quick-js/ww/a ├> /quick-js/ww/a ╰>> /quick-js/ww/a ╭<parent> /hello world/banana/ ├<child> ../../quick-js/ww/a ├> ╰>> ╭<parent> /hello world/banana/ ├<child> ./../.././quick-js/ww/a ├> ╰>> ╭<parent> /hello world/banana/ ├<child> quick-js/../quick-js/ww/a ├> /hello world/banana/quick-js/ww/a ╰>> /hello world/banana/ww/a ╭<parent> /hello world/banana/ ├<child> quick-js/./.././ww/a ├> /hello world/banana/ww/a ╰>> /hello world/banana/ww/a ╭<parent> /a/ ├<child> b../ ├> /a/ ╰>> /a/b../ ret = 4 test_resolver-paths: ../test/marginal/resolver-paths.cpp:33: int main(): Assertion `ret=0' failed. ```

╭<parent> /a/ ├<child> b../ ├> /a/b../ ╰>> /a/b../

andy5995 · 2024-12-09T10:51:18Z

This is ready for review. :)

andy5995 · 2024-12-09T10:54:49Z

src/utils/paths.cpp

+            if (j > 0) {
+                if (child[j-1] != '/') {
+                    uint8_t i = 0;
+                    for(;i<=sizeof "../";i++) {


Ok, so this you might laugh at this. ;) Something just didn't feel right about using "3" here.

In my own code, I might #define DOUBLE_DOT_DIR_SEP "../" or something at the top, then just use that string... and then using sizeof would seem more appropriate, but that too seems like overkill.

No, that is totally fine, hardcoded numbers are just bad. That is resolved at compile time so it is not like it reduces performance or anything.

For context, in modern C++ one would define a constexpr value in place of a define if it had to be made more explicit. But it is fine as it is.

Awesome! Maybe @Jammyjamjamman is right and I'm not a bad developer! :)

KaruroChori · 2024-12-09T10:58:22Z

I will be checking and stress testing it a bit. Thank you!

KaruroChori · 2024-12-14T06:56:49Z

More tests added in https://github.com/KaruroChori/vs-fltk/tree/test-path show the revised implementation failing.

andy5995 force-pushed the tests/print-ret-resolver-paths branch from b5989c3 to 606c981 Compare November 28, 2024 06:57

andy5995 force-pushed the tests/print-ret-resolver-paths branch from 606c981 to d74c2d5 Compare November 28, 2024 08:05

andy5995 force-pushed the tests/print-ret-resolver-paths branch from d74c2d5 to e02890d Compare December 5, 2024 05:12

andy5995 commented Dec 5, 2024

View reviewed changes

andy5995 marked this pull request as draft December 5, 2024 16:58

andy5995 force-pushed the tests/print-ret-resolver-paths branch from dd053e5 to e37120d Compare December 8, 2024 08:30

KaruroChori force-pushed the master branch 8 times, most recently from ee9c6af to a3bed67 Compare December 8, 2024 09:49

andy5995 added 3 commits December 9, 2024 02:38

Fix the last failure

ca57f2d

╭<parent> /a/ ├<child> b../ ├> /a/b../ ╰>> /a/b../

Execute tests in CI

fe08deb

andy5995 force-pushed the tests/print-ret-resolver-paths branch from e37120d to fe08deb Compare December 9, 2024 10:43

andy5995 marked this pull request as ready for review December 9, 2024 10:43

andy5995 added 2 commits December 9, 2024 04:45

Remove unused code block/condition

63b3002

remove leading whitespace

3ea69a0

andy5995 commented Dec 9, 2024

View reviewed changes

andy5995 changed the title ~~tests: Add debugging output to resolver-paths~~ Fix path resolver and add debugging output Dec 9, 2024

Remove unneeded block (merged with prior condition)

3f811e0

KaruroChori force-pushed the master branch from 6bde2b7 to 52a188b Compare December 11, 2024 20:54

KaruroChori mentioned this pull request Dec 14, 2024

Missing workflows for distribution #48

Open

4 tasks

KaruroChori force-pushed the master branch 14 times, most recently from 61bd0a0 to 6f6c830 Compare December 19, 2024 15:25

KaruroChori force-pushed the master branch 7 times, most recently from 4cf76d6 to ad74fb3 Compare January 23, 2025 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix path resolver and add debugging output #21

Fix path resolver and add debugging output #21

andy5995 commented Nov 28, 2024

andy5995 commented Nov 28, 2024

andy5995 commented Nov 28, 2024

KaruroChori commented Nov 28, 2024 •

edited

Loading

andy5995 commented Nov 30, 2024 •

edited

Loading

andy5995 Dec 5, 2024

KaruroChori Dec 5, 2024

andy5995 Dec 5, 2024

andy5995 commented Dec 5, 2024

KaruroChori commented Dec 5, 2024 •

edited

Loading

andy5995 commented Dec 9, 2024

andy5995 Dec 9, 2024 •

edited

Loading

KaruroChori Dec 9, 2024 •

edited

Loading

andy5995 Dec 9, 2024

KaruroChori commented Dec 9, 2024

KaruroChori commented Dec 14, 2024 •

edited

Loading

Fix path resolver and add debugging output #21

Are you sure you want to change the base?

Fix path resolver and add debugging output #21

Conversation

andy5995 commented Nov 28, 2024

andy5995 commented Nov 28, 2024

andy5995 commented Nov 28, 2024

KaruroChori commented Nov 28, 2024 • edited Loading

andy5995 commented Nov 30, 2024 • edited Loading

andy5995 Dec 5, 2024

Choose a reason for hiding this comment

KaruroChori Dec 5, 2024

Choose a reason for hiding this comment

andy5995 Dec 5, 2024

Choose a reason for hiding this comment

andy5995 commented Dec 5, 2024

KaruroChori commented Dec 5, 2024 • edited Loading

andy5995 commented Dec 9, 2024

andy5995 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

KaruroChori Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

andy5995 Dec 9, 2024

Choose a reason for hiding this comment

KaruroChori commented Dec 9, 2024

KaruroChori commented Dec 14, 2024 • edited Loading

KaruroChori commented Nov 28, 2024 •

edited

Loading

andy5995 commented Nov 30, 2024 •

edited

Loading

KaruroChori commented Dec 5, 2024 •

edited

Loading

andy5995 Dec 9, 2024 •

edited

Loading

KaruroChori Dec 9, 2024 •

edited

Loading

KaruroChori commented Dec 14, 2024 •

edited

Loading