Skip to content
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 report pathing and re-run tests #175

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Conversation

paulsbruce
Copy link
Collaborator

What?

Fix issue with path separators as reported by Issue #174

Why?

On Windows machines, path separator wasn't being taken into account as a backslash.

How?

Check all system path separators in relative resource URIs for report templates so that no matter which system it is run on, the resource namespace is derived from the relative path (i.e. tests/resources/jinja/builtin_transactions_csv.j2) properly.

Testing

Installed on a Win2016 server and verified working now manually. All other unit tests passing. Also, updated two docker-based tests to be run under the same [integration] model as the other tests which rely on Docker, so that unit test suite passes if Docker is not installed or running.

Additional Notes

This is another thing that would be caught by unit and integration tests if we start using an additional Windows build node in the build and test workflows. Closes #174

@paulsbruce paulsbruce added the bug Something isn't working label Apr 20, 2021
neoload/commands/report.py Outdated Show resolved Hide resolved
tests/commands/docker/test_docker_cleanups.py Show resolved Hide resolved
tests/commands/docker/test_docker_hooks.py Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Apr 20, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@guillaumebert guillaumebert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for this patch to get files from package on Windows, to resolve customer issue.
There is still a technical debt because the whole test folder should not be included in the released package. To Fix later

@guillaumebert guillaumebert merged commit ea37d52 into master Apr 21, 2021
@guillaumebert guillaumebert deleted the FixWinResourcePaths branch April 21, 2021 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when creating csv file
2 participants