You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@baremaps.apache.org by "leonardcser (via GitHub)" <gi...@apache.org> on 2023/05/17 11:55:20 UTC
[GitHub] [incubator-baremaps] leonardcser opened a new pull request, #659: Feature/test-renderer
leonardcser opened a new pull request, #659:
URL: https://github.com/apache/incubator-baremaps/pull/659
> Linked to issue #657
Create new integration test renderer cli.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] bchapuis merged pull request #659: Feature/test-renderer
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis merged PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] leonardcser commented on pull request #659: Feature/test-renderer
Posted by "leonardcser (via GitHub)" <gi...@apache.org>.
leonardcser commented on PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#issuecomment-1554259145
I am seeing that often tests fails because of the icon placement. @bchapuis Are the position of the icons non deterministic?
<img width="1373" alt="Screenshot 2023-05-19 at 10 56 27" src="https://github.com/apache/incubator-baremaps/assets/73912641/191ec0b2-d5b3-4796-ae7a-56675f312795">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] bchapuis commented on pull request #659: Feature/test-renderer
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#issuecomment-1552892588
The code is well organized, well documented and reads really well! This is a great contribution, thank you.
A minor comment regarding the CLI, it failed when I tried to use absolute paths:
```
ERROR: Folder '/Users/bchapuis/Projects/apache/baremaps/basemap/tests/integration' could not be found in current directory
```
Also, I expected to see some images poping in the test directory, and noticed that, in case of success, the files were cleaned. Personnally, I would prefer to always see the files by default.
Finally, I think it would make sense to move the tool to the root directory and have if along side the other modules. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] leonardcser commented on a diff in pull request #659: Feature/test-renderer
Posted by "leonardcser (via GitHub)" <gi...@apache.org>.
leonardcser commented on code in PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#discussion_r1199012995
##########
baremaps-renderer/assets/report-template.html:
##########
@@ -0,0 +1,334 @@
+<!--
+ Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ in compliance with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software distributed under the License
+ is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ or implied. See the License for the specific language governing permissions and limitations under
+ the License.
+ -->
+<!DOCTYPE html>
+<html>
+ <head>
+ <title>Baremaps Renderer</title>
+ <style>
+ :root {
+ --color-pass: rgb(40, 156, 40);
+ --color-pass-transparent: rgba(40, 156, 40, 0.05);
+ --color-error: rgb(255, 59, 59);
+ --color-error-transparent: rgba(255, 59, 59, 0.05);
+ }
+
+ body {
+ font-family: sans-serif;
+ padding: 4rem 8rem;
+ }
+
+ @media (max-width: 768px) {
+ body {
+ padding: 4rem 0.5rem;
+ }
+
+ .results {
+ gap: 0.5rem !important;
+ }
+ }
+
+ .title {
+ margin-bottom: 3rem;
+ }
+
+ h1 {
+ font-size: 3rem;
+ margin-top: 0;
+ margin-bottom: 1rem;
+ }
+
+ h2 {
+ font-size: 1.5rem;
+ margin-top: 0;
+ margin-bottom: 1rem;
+ text-transform: uppercase;
+ font-weight: 500;
+ }
+
+ h3 {
+ font-size: 2rem;
+ margin: 1rem 0;
+ }
+
+ h4 {
+ font-size: 1.25rem;
+ margin-bottom: 0;
+ }
+
+ h4.fail {
+ margin-top: 0;
+ text-transform: uppercase;
+ color: var(--color-error);
+ }
+
+ h4.pass {
+ margin-top: 0;
+ text-transform: uppercase;
+ color: var(--color-pass);
+ }
+
+ a {
+ position: relative;
+ text-decoration: underline;
+ color: inherit;
+ }
+
+ pre {
+ font-size: 1.25rem;
+ }
+
+ button {
+ border: none;
+ background-color: transparent;
+ cursor: pointer;
+ font-size: 1rem;
+ padding: 0.25rem 0.5rem;
+ border-radius: 0.25rem;
+ box-shadow: 0 0 0 2px rgba(0, 0, 0, 0.075);
+ }
+
+ .sort {
+ display: flex;
+ align-items: center;
+ gap: 0.5rem;
+ margin-bottom: 1rem;
+ }
+
+ .sort p {
+ margin-right: 0.5rem;
+ }
+
+ .sort button.active {
+ box-shadow: 0 0 0 2px rgba(0, 0, 0, 0.1);
+ background-color: rgba(0, 0, 0, 0.075);
+ }
+
+ .results {
+ display: flex;
+ flex-direction: column;
+ gap: 2rem;
+ }
+
+ .summary {
+ border-left: 3px solid black;
+ padding: 1rem 2rem;
+ background-color: rgba(0, 0, 0, 0.05);
+ }
+
+ .summary h4 {
+ text-transform: uppercase;
+ margin-top: 1rem;
+ }
+
+ .p-pass {
+ color: var(--color-pass);
+ font-weight: bold;
+ }
+
+ .p-fail {
+ color: var(--color-error);
+ font-weight: bold;
+ }
+
+ .result {
+ padding: 1rem 2rem;
+ }
+
+ .result.fail {
+ border-left: 3px solid var(--color-error);
+ background-color: var(--color-error-transparent);
+ }
+
+ .result.pass {
+ border-left: 3px solid var(--color-pass);
+ background-color: var(--color-pass-transparent);
+ }
+
+ pre {
+ margin: 0;
+ }
+
+ .images {
+ display: grid;
+ grid-template-columns: 1fr 1fr 1fr;
+ gap: 1rem;
+ }
+
+ img {
+ width: 100%;
+ aspect-ratio: 1;
+ }
+ </style>
+ </head>
+ <body>
+ <div class="title">
+ <h1>Baremaps Renderer</h1>
+ <h2>Integration Testing Report</h2>
+ </div>
+ <div>
+ <!-- Sort by test status, test diff, test name -->
+ <div class="sort">
+ <p><strong>Sort by:</strong></p>
+ <button onClick="sortBy(this, 'status')">Test status</button>
+ <button onClick="sortBy(this, 'diff')">Test diff</button>
+ <button onClick="sortBy(this, 'name')">Test name</button>
+ </div>
+ </div>
+ <!-- Test results container (incl. the summary) -->
+ <div class="results" id="testResults"></div>
+ </body>
+ <script>
+ /** Template for the test summary */
+ const SUMMARY_TEMPLATE = `<div class="summary">
+<h4>Summary</h4>
+<p>Out of <strong>{{ TOT_TESTS }}</strong> tests:</p>
+<ul>
+ <li><span class="p-pass">{{ TOT_PASS_TESTS }} tests passed</span></li>
+ <li><span class="p-fail">{{ TOT_FAIL_TESTS }} tests failed</span></li>
+</ul>
+</div>`;
+
+ /**
+ * Helper function to get the test template
+ *
+ * @param {boolean} passed - Whether the test passed or not
+ * @param {number} index - The index of the test
+ * @returns {string} - The test template
+ */
+ const getTestTemplate = (passed, index) => {
+ const loading = index > 2 ? 'lazy' : 'eager';
+ return `<div class="result ${passed ? 'pass' : 'fail'}">
+ <h4 class="${passed ? 'pass' : 'fail'}">${passed ? 'PASSED' : 'FAILED'}</h4>
+ <h3>{{ TEST_NAME }}</h3>
+ <p>
+ {{ TEST_PATH }}
+ </p>
+ <pre>
+ <code>
+{{ METADATA }}</code>
+ </pre>
+ <div class="images">
+ <h4>Expected</h4>
+ <h4>Actual</h4>
+ <h4>Difference ({{ DIFF }})</h4>
+ <img src="{{ EXPECTED_IMG_PATH }}" loading="${loading}" />
+ <img src="{{ ACTUAL_IMG_PATH }}" loading="${loading}" />
+ <img src="{{ DIFF_IMG_PATH }}" loading="${loading}" />
+ </div>
+</div>`;
+ };
+
+ const testData = JSON.parse(`{{ TESTS_DATA }}`);
+
+ /**
+ * Updates the sort parameters and re-renders the test results
+ *
+ * @param {HTMLElement} el - The element that was clicked
+ * @param {string} key - The key to sort by
+ */
+ const sortBy = (el, key) => {
+ if (!sortedBy[key].active) {
+ el.classList.add('active');
+ el.innerText = `${el.innerText} ▲`;
+ sortedBy[key].asc = true;
+ sortedBy[key].active = true;
+ } else if (sortedBy[key].active && sortedBy[key].asc) {
+ el.classList.remove('asc');
+ el.innerText = `${el.innerText.replace(' ▲', '')} ▼`;
+ sortedBy[key].asc = false;
+ } else if (sortedBy[key].active && !sortedBy[key].asc) {
+ el.classList.remove('active');
+ el.innerText = el.innerText.replace(' ▼', '');
+ sortedBy[key].active = false;
+ }
+ render();
+ };
+
+ let sortedBy = {
+ status: {
+ asc: false,
+ active: false,
+ },
+ diff: {
+ asc: false,
+ active: false,
+ },
+ name: {
+ asc: false,
+ active: false,
+ },
+ };
+
+ /** Render the test results */
+ const render = () => {
+ const testResultsDiv = document.getElementById('testResults');
+ testResultsDiv.innerHTML = '';
+ const summary = document.createElement('div');
+ summary.innerHTML = SUMMARY_TEMPLATE.replace(
+ '{{ TOT_TESTS }}',
+ testData.length,
+ )
+ .replace(
+ '{{ TOT_PASS_TESTS }}',
+ testData.filter((test) => test.success).length,
+ )
+ .replace(
+ '{{ TOT_FAIL_TESTS }}',
+ testData.filter((test) => !test.success).length,
+ );
+ testResultsDiv.appendChild(summary);
+ sortedTestData = [...testData];
Review Comment:
Declaration is on line 230.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] leonardcser commented on pull request #659: Feature/test-renderer
Posted by "leonardcser (via GitHub)" <gi...@apache.org>.
leonardcser commented on PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#issuecomment-1554216160
> A minor comment regarding the CLI, it failed when I tried to use absolute paths:
Support for absolute paths can be added.
> Also, I expected to see some images poping in the test directory, and noticed that, in case of success, the files were cleaned. Personnally, I would prefer to always see the files by default.
In this case, what should the expected behaviour be like? Keep the old failing images, replace them with two identical, should the diff be replaced with a blank image?
I suggest that if a tests succeeds, we can save one of the two `actual.png` or `expected.png` (as these would be both identical).
> Finally, I think it would make sense to move the baremaps-renderer to the root directory and have it along side the other modules. What do you think?
I agree and it make more sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] bchapuis commented on pull request #659: Feature/test-renderer
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#issuecomment-1556247285
Thanks again for this contribution, I think it will be very usefull for improving the basemap!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] bchapuis commented on pull request #659: Feature/test-renderer
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#issuecomment-1554439553
> I am seeing that often tests fails because of the icon placement. @bchapuis Are the position of the icons non deterministic?
I guess it is not, the decision to display is usually made on the surrounding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] leonardcser commented on pull request #659: Feature/test-renderer
Posted by "leonardcser (via GitHub)" <gi...@apache.org>.
leonardcser commented on PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#issuecomment-1554484955
@bchapuis This is ready for review
> > I am seeing that often tests fails because of the icon placement. @bchapuis Are the position of the icons non deterministic?
>
> I guess it is not, the decision to display is usually made on the surrounding.
In that case, for now it can be ignore visually when checking the report.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-baremaps] github-code-scanning[bot] commented on a diff in pull request #659: Feature/test-renderer
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #659:
URL: https://github.com/apache/incubator-baremaps/pull/659#discussion_r1198994176
##########
baremaps-renderer/assets/report-template.html:
##########
@@ -0,0 +1,334 @@
+<!--
+ Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ in compliance with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software distributed under the License
+ is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ or implied. See the License for the specific language governing permissions and limitations under
+ the License.
+ -->
+<!DOCTYPE html>
+<html>
+ <head>
+ <title>Baremaps Renderer</title>
+ <style>
+ :root {
+ --color-pass: rgb(40, 156, 40);
+ --color-pass-transparent: rgba(40, 156, 40, 0.05);
+ --color-error: rgb(255, 59, 59);
+ --color-error-transparent: rgba(255, 59, 59, 0.05);
+ }
+
+ body {
+ font-family: sans-serif;
+ padding: 4rem 8rem;
+ }
+
+ @media (max-width: 768px) {
+ body {
+ padding: 4rem 0.5rem;
+ }
+
+ .results {
+ gap: 0.5rem !important;
+ }
+ }
+
+ .title {
+ margin-bottom: 3rem;
+ }
+
+ h1 {
+ font-size: 3rem;
+ margin-top: 0;
+ margin-bottom: 1rem;
+ }
+
+ h2 {
+ font-size: 1.5rem;
+ margin-top: 0;
+ margin-bottom: 1rem;
+ text-transform: uppercase;
+ font-weight: 500;
+ }
+
+ h3 {
+ font-size: 2rem;
+ margin: 1rem 0;
+ }
+
+ h4 {
+ font-size: 1.25rem;
+ margin-bottom: 0;
+ }
+
+ h4.fail {
+ margin-top: 0;
+ text-transform: uppercase;
+ color: var(--color-error);
+ }
+
+ h4.pass {
+ margin-top: 0;
+ text-transform: uppercase;
+ color: var(--color-pass);
+ }
+
+ a {
+ position: relative;
+ text-decoration: underline;
+ color: inherit;
+ }
+
+ pre {
+ font-size: 1.25rem;
+ }
+
+ button {
+ border: none;
+ background-color: transparent;
+ cursor: pointer;
+ font-size: 1rem;
+ padding: 0.25rem 0.5rem;
+ border-radius: 0.25rem;
+ box-shadow: 0 0 0 2px rgba(0, 0, 0, 0.075);
+ }
+
+ .sort {
+ display: flex;
+ align-items: center;
+ gap: 0.5rem;
+ margin-bottom: 1rem;
+ }
+
+ .sort p {
+ margin-right: 0.5rem;
+ }
+
+ .sort button.active {
+ box-shadow: 0 0 0 2px rgba(0, 0, 0, 0.1);
+ background-color: rgba(0, 0, 0, 0.075);
+ }
+
+ .results {
+ display: flex;
+ flex-direction: column;
+ gap: 2rem;
+ }
+
+ .summary {
+ border-left: 3px solid black;
+ padding: 1rem 2rem;
+ background-color: rgba(0, 0, 0, 0.05);
+ }
+
+ .summary h4 {
+ text-transform: uppercase;
+ margin-top: 1rem;
+ }
+
+ .p-pass {
+ color: var(--color-pass);
+ font-weight: bold;
+ }
+
+ .p-fail {
+ color: var(--color-error);
+ font-weight: bold;
+ }
+
+ .result {
+ padding: 1rem 2rem;
+ }
+
+ .result.fail {
+ border-left: 3px solid var(--color-error);
+ background-color: var(--color-error-transparent);
+ }
+
+ .result.pass {
+ border-left: 3px solid var(--color-pass);
+ background-color: var(--color-pass-transparent);
+ }
+
+ pre {
+ margin: 0;
+ }
+
+ .images {
+ display: grid;
+ grid-template-columns: 1fr 1fr 1fr;
+ gap: 1rem;
+ }
+
+ img {
+ width: 100%;
+ aspect-ratio: 1;
+ }
+ </style>
+ </head>
+ <body>
+ <div class="title">
+ <h1>Baremaps Renderer</h1>
+ <h2>Integration Testing Report</h2>
+ </div>
+ <div>
+ <!-- Sort by test status, test diff, test name -->
+ <div class="sort">
+ <p><strong>Sort by:</strong></p>
+ <button onClick="sortBy(this, 'status')">Test status</button>
+ <button onClick="sortBy(this, 'diff')">Test diff</button>
+ <button onClick="sortBy(this, 'name')">Test name</button>
+ </div>
+ </div>
+ <!-- Test results container (incl. the summary) -->
+ <div class="results" id="testResults"></div>
+ </body>
+ <script>
+ /** Template for the test summary */
+ const SUMMARY_TEMPLATE = `<div class="summary">
+<h4>Summary</h4>
+<p>Out of <strong>{{ TOT_TESTS }}</strong> tests:</p>
+<ul>
+ <li><span class="p-pass">{{ TOT_PASS_TESTS }} tests passed</span></li>
+ <li><span class="p-fail">{{ TOT_FAIL_TESTS }} tests failed</span></li>
+</ul>
+</div>`;
+
+ /**
+ * Helper function to get the test template
+ *
+ * @param {boolean} passed - Whether the test passed or not
+ * @param {number} index - The index of the test
+ * @returns {string} - The test template
+ */
+ const getTestTemplate = (passed, index) => {
+ const loading = index > 2 ? 'lazy' : 'eager';
+ return `<div class="result ${passed ? 'pass' : 'fail'}">
+ <h4 class="${passed ? 'pass' : 'fail'}">${passed ? 'PASSED' : 'FAILED'}</h4>
+ <h3>{{ TEST_NAME }}</h3>
+ <p>
+ {{ TEST_PATH }}
+ </p>
+ <pre>
+ <code>
+{{ METADATA }}</code>
+ </pre>
+ <div class="images">
+ <h4>Expected</h4>
+ <h4>Actual</h4>
+ <h4>Difference ({{ DIFF }})</h4>
+ <img src="{{ EXPECTED_IMG_PATH }}" loading="${loading}" />
+ <img src="{{ ACTUAL_IMG_PATH }}" loading="${loading}" />
+ <img src="{{ DIFF_IMG_PATH }}" loading="${loading}" />
+ </div>
+</div>`;
+ };
+
+ const testData = JSON.parse(`{{ TESTS_DATA }}`);
+
+ /**
+ * Updates the sort parameters and re-renders the test results
+ *
+ * @param {HTMLElement} el - The element that was clicked
+ * @param {string} key - The key to sort by
+ */
+ const sortBy = (el, key) => {
+ if (!sortedBy[key].active) {
+ el.classList.add('active');
+ el.innerText = `${el.innerText} ▲`;
+ sortedBy[key].asc = true;
+ sortedBy[key].active = true;
+ } else if (sortedBy[key].active && sortedBy[key].asc) {
+ el.classList.remove('asc');
+ el.innerText = `${el.innerText.replace(' ▲', '')} ▼`;
+ sortedBy[key].asc = false;
+ } else if (sortedBy[key].active && !sortedBy[key].asc) {
+ el.classList.remove('active');
+ el.innerText = el.innerText.replace(' ▼', '');
+ sortedBy[key].active = false;
+ }
+ render();
+ };
+
+ let sortedBy = {
+ status: {
+ asc: false,
+ active: false,
+ },
+ diff: {
+ asc: false,
+ active: false,
+ },
+ name: {
+ asc: false,
+ active: false,
+ },
+ };
+
+ /** Render the test results */
+ const render = () => {
+ const testResultsDiv = document.getElementById('testResults');
+ testResultsDiv.innerHTML = '';
+ const summary = document.createElement('div');
+ summary.innerHTML = SUMMARY_TEMPLATE.replace(
+ '{{ TOT_TESTS }}',
+ testData.length,
+ )
+ .replace(
+ '{{ TOT_PASS_TESTS }}',
+ testData.filter((test) => test.success).length,
+ )
+ .replace(
+ '{{ TOT_FAIL_TESTS }}',
+ testData.filter((test) => !test.success).length,
+ );
+ testResultsDiv.appendChild(summary);
+ sortedTestData = [...testData];
Review Comment:
## Missing variable declaration
Variable sortedTestData is used like a local variable, but is missing a declaration.
[Show more details](https://github.com/apache/incubator-baremaps/security/code-scanning/662)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@baremaps.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org