Add & fix SVG Graphics Context unit tests (PR #26504)
Blake-Madden
Basically, a total overhaul of the wxSVGFileDC unit tests. I was getting failing tests with test_drawing and the image test case would only work on Windows. I did a rewrite from CppUnit to Catch2 and removed the external file dependencies. I also removed the "plugin" test interface in here. It seems to have never been used and is orphaned in numerous places. I removed it from here; let me know if you want me to remove the remaining traces of this.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26504
Commit Summary
- 9eff7ff Overhaul SVG unit tests
- 14e502d Fix missing shape-rendering attribute in SVG graphics groups
File Changes
(5 files)
- M src/common/dcsvg.cpp (4)
- M tests/drawing/basictest.cpp (17)
- M tests/drawing/drawing.cpp (523)
- M tests/drawing/drawing.h (174)
- M tests/drawing/fonttest.cpp (31)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26504.patch
- https://github.com/wxWidgets/wxWidgets/pull/26504.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- a41df2c Fix CppUnit shim
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26504)Just checking: was this closed intentionally?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
Blake-Madden left a comment (wxWidgets/wxWidgets#26504)Yes, I was getting a bunch of warnings as errors from CppUnit in other files in some of the build configurations here. No idea why though, was coming from files I hadn't touched. Catch2'ifying the test_drawing target broke something somehow, so I'll either have to rethink this (or finish removing CppUnit from everyone else first).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26504)Looking at the first failing build, the errors are due to wxBrush being incomplete, almost certainly due to the fact that wx/brush.h was indirectly included before but needs to be explicitly included now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- e5f5a40 Add another missing include
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 45b5420 Delete orphaned plugin unit testing feature
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26504)I'm not sure about the "plugin" tests but I'd definitely prefer to remove it one independent commit before making the SVG-related changes. Could you do it like this please?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Blake-Madden
Blake-Madden left a comment (wxWidgets/wxWidgets#26504)I'm not sure about the "plugin" tests but I'd definitely prefer to remove it one independent commit before making the SVG-related changes. Could you do it like this please?
See #26568
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26504)This has conflicts now, do you plan on resolving them or should I (try to) do it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
Blake-Madden left a comment (wxWidgets/wxWidgets#26504)I'll take a look at it tonight
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- d037a10 Merge branch 'master' into SVGTests
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 2 commits.
- f605d2b Remove all CppUnit code after rebase
- 0e218b0 Remove reference imaged used in old comparisons
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- a8d6ecd Add missing header
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- cbc2740 Fix more missing includes
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 82fe01d Fix include guards
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
Blake-Madden left a comment (wxWidgets/wxWidgets#26504)Removed all CppUnit and old image comparison content. Tests pass for me now, although it doesn't appear that our CI here is running test_drawing. Not sure if that is wanted or not, but probably just a matter of adding it to the matrix in ci.yml.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
There are a lot of whitespace only and otherwise insignificant (e.g. additions of wxASCII_STR, removal of comments, ...) changes here so it's really difficult to see what has changed, i.e. what exactly has been fixed?
We do need to run these tests if we want to preserve/keep them going forward, so we should do it in at least some CI jobs, could you please add running them there?
> @@ -1525,11 +1526,12 @@ void wxSVGFileDCImpl::DoStartNewGraphics()
if ( penStroke.empty() )
penStroke = GetPenStroke(m_pen.GetColour(), m_pen.GetStyle());
- s = wxString::Format(wxS("<g %s %s %s %s transform=\"translate(%d %d) scale(%s %s)\">\n"),
+ s = wxString::Format(wxS("<g %s %s %s %s %s transform=\"translate(%d %d) scale(%s %s)\">\n"),
I'm not sure how this works, but it looks like we might be outputting shape-rendering="auto" here unnecessarily, shouldn't we only add this if the rendering mode is different from default?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 2 commits.
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> @@ -1525,11 +1526,12 @@ void wxSVGFileDCImpl::DoStartNewGraphics()
if ( penStroke.empty() )
penStroke = GetPenStroke(m_pen.GetColour(), m_pen.GetStyle());
- s = wxString::Format(wxS("<g %s %s %s %s transform=\"translate(%d %d) scale(%s %s)\">\n"),
+ s = wxString::Format(wxS("<g %s %s %s %s %s transform=\"translate(%d %d) scale(%s %s)\">\n"),
fixed
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
Blake-Madden left a comment (wxWidgets/wxWidgets#26504)I added test_drawing to the CI runs.
Sorry, what was more of a total rewrite than changing things. I don't think it can be meaningful compared to the old test harness, but rather reviewed as all new code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 69ed7d8 test_drawing needs to be excluded from make CI
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Blake-Madden
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.