Conversation
…hrough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…hrough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…hrough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ps://github.com/open-edge-platform/scenescape into feature/ITEP-83148-UI-mechanism-to-upload-video
…eature/ITEP-77648-mesh-creation-UI-test
Co-authored-by: Sarat Poluri <sarat.chandra.poluri@intel.com>
…eature/ITEP-77648-mesh-creation-UI-test
daddo-intel
changed the base branch from
main
to
feature/ITEP-83148-UI-mechanism-to-upload-video
…thub.com/open-edge-platform/scenescape into feature/ITEP-77648-mesh-creation-UI-test
|
Not going to get it in for RC1 |
There was a problem hiding this comment.
Pull request overview
Adds a new Selenium-based UI test target to exercise the mesh creation workflow, along with supporting test-compose and container readiness tweaks.
Changes:
- Introduces
tests/ui/tc_mesh_creation.pyand amake -C tests mesh-creationtarget to run it. - Adds a dedicated
tests/compose/mapping.ymland updates container wait logic for the mapping service. - Updates
manager/config/scenescape-initwith new workspace uid/gid alignment and HOSTDIR bind-mount behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/ui/tc_mesh_creation.py |
New UI test that drives the “Generate Mesh” flow from cameras and from an uploaded video. |
tests/runtest |
Adds mapping-specific wait log text and timeout to support the new compose service. |
tests/compose/mapping.yml |
New test-compose definition for bringing up the mapping service during UI tests. |
tests/Makefile.user_interface |
Adds the mesh-creation make target wiring the new test + compose stack. |
manager/config/scenescape-init |
Adds uid/gid alignment + new HOSTDIR/root gating logic that affects runtime init behavior. |
| for attempt in range(MAX_ATTEMPTS): | ||
| found = common.wait_for_elements(browser, "generate_mesh", findBy=By.ID) | ||
| if found: | ||
| break | ||
| browser.refresh() | ||
| time.sleep(1) | ||
|
|
There was a problem hiding this comment.
time.sleep(1) is used but time is never imported in this test file, which will raise NameError and fail the test at runtime. Add import time (and keep imports minimal/ordered to match other UI tests).
| def create_mesh_from_cameras(browser): | ||
| click_generate_mesh(browser) | ||
| try: | ||
| alert = WebDriverWait(browser, 60).until(EC.alert_is_present()) | ||
| alert_text = alert.text | ||
| print(alert_text) | ||
| assert alert_text == 'Mesh generated successfully! The scene map has been updated.' | ||
| alert.accept() | ||
| except TimeoutException: | ||
| print("No alert appeared") | ||
| return | ||
|
|
||
| def create_mesh_from_video(browser, video_file): | ||
| browser.refresh() | ||
| browser.find_element(By.ID, "id_map").send_keys(video_file) | ||
| click_generate_mesh(browser) | ||
| try: | ||
| alert = WebDriverWait(browser, 60 * 10).until(EC.alert_is_present()) | ||
| alert_text = alert.text | ||
| assert alert_text == 'Mesh generated successfully! The scene map has been updated.' | ||
| alert.accept() | ||
| except TimeoutException: | ||
| print("No alert appeared") | ||
| return |
There was a problem hiding this comment.
Both create_mesh_from_cameras and create_mesh_from_video swallow TimeoutException and continue, meaning the test can report success even if the success alert never appears (false positive). The timeout path should fail the test (e.g., by raising/asserting) so mesh generation is actually validated.
| """! Checks that the camera parameters in the web UI can be updated and | ||
| that they persist after saving, for both Camera Save buttons. | ||
| @param params Dict of test parameters. | ||
| @param record_xml_attribute Pytest fixture recording the test name. | ||
| @return exit_code Indicates test success or failure. | ||
| """ |
There was a problem hiding this comment.
The test function docstring appears to be copy/pasted from a camera-parameter test (it describes updating camera parameters/persistence). Please update it to describe the mesh creation flow being tested so the test intent and reporting are accurate.
| from urllib.parse import urlparse | ||
| import os |
There was a problem hiding this comment.
urlparse and os are imported but unused in this test, and import os has trailing whitespace. Please remove unused imports/whitespace to keep the UI test suite clean and avoid lint/format failures.
| from urllib.parse import urlparse | |
| import os |
| if [ "$(id -u)" -ne 0 ]; then | ||
| echo "ERROR: HOSTDIR bind-mount requires root. Current UID=$(id -u)." | ||
| echo "Hint: run container as root (docker run -u root) or enable super-shell." | ||
| exit 1 | ||
| fi | ||
|
|
||
| mkdir -p "${HOSTDIR}" | ||
| mount --bind "${WORKSPACE}" "${HOSTDIR}" | ||
|
|
There was a problem hiding this comment.
This new HOSTDIR bind-mount path now hard-fails unless the container is running as UID 0. tools/scenescape-start runs the manager image as the non-root image user by default, and the script previously relied on CAP_SYS_ADMIN/privileged to perform the bind mount. As written, starting the manager with HOSTDIR set will now exit early. Consider removing the strict UID==0 check (or switching to a mechanism that works with capabilities), or update the startup tooling to run this init as root and then drop privileges afterward.
| if [ "$(id -u)" -ne 0 ]; then | |
| echo "ERROR: HOSTDIR bind-mount requires root. Current UID=$(id -u)." | |
| echo "Hint: run container as root (docker run -u root) or enable super-shell." | |
| exit 1 | |
| fi | |
| mkdir -p "${HOSTDIR}" | |
| mount --bind "${WORKSPACE}" "${HOSTDIR}" | |
| mkdir -p "${HOSTDIR}" | |
| if ! mount --bind "${WORKSPACE}" "${HOSTDIR}"; then | |
| echo "ERROR: Failed to perform HOSTDIR bind-mount from ${WORKSPACE} to ${HOSTDIR} (UID=$(id -u))." | |
| if [ "$(id -u)" -ne 0 ]; then | |
| echo "Hint: run container as root (docker run -u root) or enable super-shell / CAP_SYS_ADMIN." | |
| fi | |
| exit 1 | |
| fi |
| groupadd -g "${WSGID}" hostgrp 2>/dev/null || true | ||
| fi | ||
|
|
||
| # Set primary group then uid | ||
| usermod -g "${WSGID}" "${WSUSER}" 2>/dev/null || true | ||
| usermod -u "${WSUID}" "${WSUSER}" 2>/dev/null || true | ||
|
|
||
| # Fix ownership of WSUSER home to avoid sudo -H issues | ||
| WSHOME=$(eval echo ~${WSUSER}) | ||
| chown -R "${WSUID}:${WSGID}" "${WSHOME}" 2>/dev/null || true |
There was a problem hiding this comment.
The uid/gid alignment section ignores failures from groupadd/usermod/chown via || true while set -e is enabled. If any of these steps fail, the container can continue in a partially-updated state that’s hard to diagnose (and may still leave bind-mounted files unwritable). Consider handling failures explicitly (log + exit, or at least log that alignment was skipped) so permission issues aren’t silently masked.
| groupadd -g "${WSGID}" hostgrp 2>/dev/null || true | |
| fi | |
| # Set primary group then uid | |
| usermod -g "${WSGID}" "${WSUSER}" 2>/dev/null || true | |
| usermod -u "${WSUID}" "${WSUSER}" 2>/dev/null || true | |
| # Fix ownership of WSUSER home to avoid sudo -H issues | |
| WSHOME=$(eval echo ~${WSUSER}) | |
| chown -R "${WSUID}:${WSGID}" "${WSHOME}" 2>/dev/null || true | |
| if ! groupadd -g "${WSGID}" hostgrp 2>/dev/null; then | |
| echo "ERROR: Failed to create group with GID ${WSGID} for workspace alignment" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| # Set primary group then uid | |
| if ! usermod -g "${WSGID}" "${WSUSER}" 2>/dev/null; then | |
| echo "ERROR: Failed to set primary group GID ${WSGID} for user ${WSUSER}" >&2 | |
| exit 1 | |
| fi | |
| if ! usermod -u "${WSUID}" "${WSUSER}" 2>/dev/null; then | |
| echo "ERROR: Failed to set UID ${WSUID} for user ${WSUSER}" >&2 | |
| exit 1 | |
| fi | |
| # Fix ownership of WSUSER home to avoid sudo -H issues | |
| WSHOME=$(eval echo ~${WSUSER}) | |
| if ! chown -R "${WSUID}:${WSGID}" "${WSHOME}" 2>/dev/null; then | |
| echo "WARN: Failed to chown ${WSHOME} to ${WSUID}:${WSGID}; home directory permissions may be inconsistent" >&2 | |
| fi |
| # --- Align WSUSER uid/gid with workspace so bind-mounted files are writable --- | ||
| if [ "$(id -u)" -eq 0 ] && [ -n "${HOSTDIR}" ]; then | ||
| echo "Workspace ownership: ${WSUID}:${WSGID}" | ||
|
|
||
| if id "${WSUSER}" >/dev/null 2>&1; then | ||
| echo "Before: ${WSUSER} -> $(id -u ${WSUSER}):$(id -g ${WSUSER})" | ||
|
|
||
| # Ensure a group with the target GID exists | ||
| if ! getent group "${WSGID}" >/dev/null 2>&1; then | ||
| groupadd -g "${WSGID}" hostgrp 2>/dev/null || true | ||
| fi | ||
|
|
||
| # Set primary group then uid | ||
| usermod -g "${WSGID}" "${WSUSER}" 2>/dev/null || true | ||
| usermod -u "${WSUID}" "${WSUSER}" 2>/dev/null || true | ||
|
|
||
| # Fix ownership of WSUSER home to avoid sudo -H issues | ||
| WSHOME=$(eval echo ~${WSUSER}) | ||
| chown -R "${WSUID}:${WSGID}" "${WSHOME}" 2>/dev/null || true | ||
|
|
||
| echo "After: ${WSUSER} -> $(id -u ${WSUSER}):$(id -g ${WSUSER})" | ||
| else | ||
| echo "WARN: user ${WSUSER} not found; cannot align uid/gid" | ||
| fi | ||
| fi | ||
|
|
||
| if [ -n "${HOSTDIR}" ] ; then | ||
| if [ "$(id -u)" -ne 0 ]; then | ||
| echo "ERROR: HOSTDIR bind-mount requires root. Current UID=$(id -u)." | ||
| echo "Hint: run container as root (docker run -u root) or enable super-shell." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The PR description/title indicates this change is only adding a UI test, but this file introduces substantial runtime-init behavior changes (uid/gid mutation, new bind-mount/root requirement). Please either document this in the PR description (why it’s needed for the mesh creation test) or split these operational changes into a separate PR for easier review/rollback.
📝 Description
This PR adds a UI test for mesh creation flow:
To run test:
make -C tests mesh-creation✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: