-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CMake test setup #112
Fix CMake test setup #112
Conversation
For completeness, here's the current output of
|
I should also mention that this does not affect the Github workflow. Those tests will still be reported as passing, though the segfault occurs. |
Overall I support your approach to simplify the code around testing. Logging should not be part of these tests.
|
Let us not start this discussion again. The author of this repo merged #107 without any comment, which indicates to me that your best practices regarding commits and pull requests might not be the same as hers. Overall, the messed up commit timeline was a result of merging your PR #109 which conflicted with mine made earlier, and then later merging #107, though I thought it was made clear that it shouldn't be merged. This is not my fault! |
I agree with @gruenich : it's better to set up individual PRs, rather than big chunk. |
And we totally agree on that. But
If you think this PR can't be merged as is, feel free to close it. |
You are right, this merge request ist not too big. Nevertheless, splitting it up into smaller commits with descriptive commit messages makes it easier to understand what happens. Further, if someone is looking for regressions, it helps with bisecting the change. |
I know exactly what you mean. I just think it's a case of overengineering. What you are basically asking is not rebasing, but resetting and completely re-doing the PR. But at the end it's just a commit fixing a completely broken CMake test setup by removing the part that is broken. To me, this doesn't need more dissection. It's not affecting the actual C code base, it's not affecting other parts of the build system - just a very simple fix for a broken setup. If @xiaoyeli doesn't agree, I'm fine with that. Close it. |
I've cleaned up the commit timeline. This eliminates the previous merge with upstream. I'm not going to dissect this further, reasons explained above. |
To finally come to a conclusion: In case @xiaoyeli thinks that logging should not be removed, I'd totally accept that as a reason to reject this PR. Since it wouldn't make sense to rework this PR, a new one should be opened to fix the test setup including the logging. In case this PR gets merged, the readme should be updated, since ctest will no longer produce the *.out log files. |
#114 was merged, containing your changes to the test setup. Thanks for providing the code! I lost track of the changes in this pull request, that where not part of #114. I still support your idea of having tests that reflect reality. I wouldn't enable failing tests, but once fixed, they should be properly reported. Let's join forces to achieve this goal and salvage the remaining part from here. |
Dear @gruenich, dear @xiaoyeli. This has probably been the worst experience contributing to an open source project - EVER! Consider this comment to be the last interaction with this repo. |
I am sorry to hear that your contribution experience feels so bad. I would hate to see losing your input and your code improvements to the SuperLU project! I tried to hand over my learning from contributing to SuperLU. I hope this helps you to understand my actions, which were not meant to be hostile:
|
Regarding bullet 1 above, our fundings/efforts in recent years have been on SuperLU_DIST, distributed memory and GPU development. |
I really tried to leave all this mess alone, but I just can't... @xiaoyeli No, no, no, no, no ... there's no excuse for completely ignoring the things I have said in all those discussions (well, rather monologues, or dialogs with @gruenich ). You have been active here during the last 3 months, answering issues, merging pull requests (introducing bugs), pushing code to master (introducing nonsense). I understand that this project has no priority for you, but that cannot justify you acting so irresponsible or you ignoring, for example, my honest and repeated advice to get the CI tests working. No excuse for that! In #108 on August 5th (yes, nearly 4 months ago now) I first mentioned that the tests are - quote - fundamentally flawed (meaning they aren't testing anything) - and that's the correct assessment. On the same day I laid out the path to fix that, opening a pull request that fixed the problems. Unfortunately, @gruenich jumped in complaining about the formal look of the PR and you supported that (the one and only comment I ever got from you), but then again completely ignoring my arguments against @gruenich complaints. So, why am I writing all this? Because I just downloaded the master branch and tried to build with Visual Studio - and it failed... Reason: e87529f Revert that and it compiles. But surprise: ctest reports ... segfaults. Reason: not sure, maybe #108 (comment) or #108 (comment), but, hey @xiaoyeli, lets ignore that... In #108 (comment) I laid out the path how to fix the Github workflow and back then I would have been happy to help out. But not anymore. So, @gruenich, since you are so keen to help here, do your thing... Final comment. EVER! (maybe not) |
WARNING: this pull request will make the tests fail due to currently unresolved issue #108
That being said, I think it's important to have tests reflecting reality and I think that this should be merged rather sooner than later (even if the issue remains unresolved for now).
As explained in above mentioned issue, the current CMake test setup is rather complex. This is due mainly to the logging implemented. This pull request removes all those logging features, which greatly simplifies the setup. After all, I think logging should not be part of automated tests - at least not on the test subject side, the testing framework should handle that, and if anybody wants verbose output, ctest can be called like
Additionally, the test executables can still be run by hand to get the test output