Comments (12)
Thank you for the recommendation.
If I understand correctly, all the classes and namespaces in ApprovalTests.cpp/ApprovalTests
would go in a ApprovalTests
namespace. I think that would help users with larger code bases, as we have some obvious names like SystemUtils
and StringUtils
which would cause problems if approvals, in its current form, were used in code-bases that already used these names.
Does that mean that CombinationApprovals::verifyAllCombinations()
would become ApprovalTests::CombinationApprovals::verifyAllCombinations()
. I presume so - I guess we would need to think about why the current class static-methods-only ApprovalTests
would be any different from any of the other static-methods-only classes.
from approvaltests.cpp.
We are intending to introduce a consistent namespace to all of the contents of ApprovalTests/ soon - likely in the next week...
We may also add a second layer of namespaces, e.g. something like ApprovalTests::Writers::StringWriter...
Comments on this would be appreciated....
from approvaltests.cpp.
I've started experimenting with this in CLion, trying to find an efficient workflow for moving all our code in ApprovalTests/
to a new ApprovalTests namespace.
I'll record here what I've found - and ruled out.
from approvaltests.cpp.
Try 1
Just use Move refactoring in CLion - gets paths of headers wrong, so won’t compile - tried a script to remove the wrong lines - too much faff. Example build error:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/tests/ApprovalTests_Catch2_Tests/CombinationTests.cpp:6:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/CombinationApprovals.h:7:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DefaultReporter.h:5:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DefaultReporterFactory.h:5:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DiffReporter.h:5:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/WindowsReporters.h:4:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DiffPrograms.h:4:
/Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/reporters/DiffInfo.h:8:10: fatal error: 'FileUtils.h' file not found
#include "FileUtils.h"
^~~~~~~~~~~~~
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp/tests/ApprovalTests_Catch2_Tests/ApprovalsTests.cpp:In file included from 4:
from approvaltests.cpp.
Try 2
In CLion, move all headers to top level of project, then use Move in CLion.
Too much faff - was quite time-consuming - and it made it harder to see the other changes.
Would also need another step to move all the files back to their original directory - and would mess up the history and Annotate/Blame too much.
from approvaltests.cpp.
Try 3
Go back to use Move refactoring in CLion - gets paths of headers wrong still, so work around that by editing CMakeLists.txt to temporarily get builds working with the new, wrong #include paths - with the intention to revert the CMake edit later, and delete all the wrong #includes in one go.
This was the first change in ApprovalTests/CMakeLists.txt - which was needed when refactoring headers in ApprovalTests - when we move on to the sub-directories, like namers
, I expect we would need to make further edits to add those directories too:
-target_include_directories(${PROJECT_NAME}
- INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/..)
+target_include_directories(${PROJECT_NAME}
+ INTERFACE
+ ${CMAKE_CURRENT_SOURCE_DIR}/..
+ # Temporary extra paths,
+ ${CMAKE_CURRENT_SOURCE_DIR}/
+ )
This went well, albeit slowly on a fast Mac, until I spotted that CLion was making needless edits to std::shared_ptr - changing it to std::__1::shared_ptr - same for std::vector.
These were hard to spot when viewing diffs, due CLion indenting the code that was moved to the namespace.
from approvaltests.cpp.
Try 4
Start again, as in previous step, but try to change CLion’s settings so that, for now, it doesn’t indent when adding namespace - so it should be easier to spot any extra edits that it makes during these refactoring steps.
from approvaltests.cpp.
Status
I ended up finishing this off by making the edits to headers by hand, then fixing build failures. It was faster than waiting for CLion's refactoring.
The changes on the namespaces_experiment branch show the progress I made on this.
(Click on the "Files Changed" tab to see the differences". I've minimised the changes, like not changing indentation, so that this work can be reviewed more easily.)
Basically, everything in the library is now in the namespace ApprovalTests. We talked about adding sub-namespaces like Reporters
. My current thinking is to not do this, as it's likely too much work for what I understand the benefits to be.
I did this before rearranging the files in the library, to not get under the feet of someone who may possibly offer us some code improvements...
Remaining code to review
About the only files not fully updated are the Catch2 and Doctest integrations. We should discuss those.
Okra integration
I also spent about an hour looking at setting up a test for the Okra integration - and then found that it has not compiled for some time because of changes we made in ApprovalTests.cpp. But more significantly, it didn't build against the version of Okra.h from the time when this integrated in to this project.
I searched all of github for uses of the ApprovalTests.cpp/Okra integration, and there were none.
I've sent @JayBazuzi a message on Twitter to see if he wants to pair on it, but right now my feeling is we should probably just archive the Okra integration code, since no-one has logged a bug that it has been broken for some time.
Other Comments
It's possible that some of the ApprovalTests::
that were added early on could now be removed - they were added in library files that had not yet been moved to the namespace.
They are harmless, and I'm not worrying about the, now.
from approvaltests.cpp.
I've updated the docs too, on the branch, to give a sense what the code looks like now:
We might want to shorten lines in the first example in the above.
The above does show the repetition - anyone who cares can do using namespace ApprovalTests;
in their code - but it would trip people up if we did this in our docs, I feel.
from approvaltests.cpp.
I don't mind if you break Okra, because I have epsilon users.
One reason you might want to keep Okra working is that have Approval Tests work with multiple frameworks helps you know your integration points are sufficiently generally... But that's up to you.
from approvaltests.cpp.
Thanks Jay, the thing is more that, as far as I can tell, the ApprovalTests integration has never worked, and I can’t tell how to fix it.
from approvaltests.cpp.
This is now done.
from approvaltests.cpp.
Related Issues (20)
- Add clang12 CI build HOT 1
- Provide a script to allow users to generate the single header from the current code, without doing a release HOT 9
- Add a documentation example showing how to obtain ApprovalTests.cpp via CPM.cmake HOT 1
- Add vcpkg to our release process HOT 2
- Document how to use Approval Tests with vcpkg HOT 1
- Bullet lists not rendered correcting on Read the Docs version of our documentation HOT 3
- Add a copyright statement to the released single header HOT 1
- GitHub Actions: Don't run scheduled tasks on forked repos
- Using kdiff3 as diff tool, user has to specify name of output file after merge
- cygwin CI build hangs for 6 hours then fails HOT 5
- warning C4459 in MSVC with /W4 HOT 1
- Does not compile with clang13 due to -Werror/-Wdeprecated-copy-with-dtor HOT 2
- Ninja + Catch2 + mingw64
- useApprovalsSubdirectory() doesn't create the output directory in the expected location HOT 3
- "Unable to create directory" - unable to run test build with mingw provided by qt-installer HOT 5
- with CppUTest: `ApprovalMismatchException` suppresses other tests output HOT 4
- add `code -d {Received} {Approved}` to Reporters list
- Catch2 integration does not handle Generators inside sections HOT 1
- Catch2 v3 integration HOT 5
- Bug with {fmt} integration HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from approvaltests.cpp.