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.

Sign up for GitHub

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

Jump to bottom

[unit-tests-only] Add todo unit tests for PrivateLink view #5750

Merged
merged 1 commit into from
Aug 31, 2021
Merged

[unit-tests-only] Add todo unit tests for PrivateLink view #5750

merged 1 commit into from
Aug 31, 2021

Conversation

saw-jan
Copy link
Member

@saw-jan saw-jan commented Aug 30, 2021

Description

Added todo unit tests for PrivateLink view

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment: local

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@saw-jan saw-jan requested a review from fschade as a code owner August 30, 2021 10:02
@saw-jan saw-jan self-assigned this Aug 30, 2021
@saw-jan saw-jan mentioned this pull request Aug 30, 2021
Closed
11 tasks
pascalwengerter
pascalwengerter reviewed Aug 30, 2021
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Looks good, please go ahead 🙌🏽

@saw-jan
Copy link
Member Author

saw-jan commented Aug 30, 2021

@pascalwengerter should I wait for this PR to be merged or add tests in this PR.
As we have discussed in an earlier meeting, we merge this PR and write actual tests in another PR

pascalwengerter
pascalwengerter requested changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Please put the top 4 todos into their own "when the page has loaded successfully" describe block but otherwise good to go!

@saw-jan saw-jan requested a review from kulmann August 31, 2021 03:25
kulmann
kulmann requested changes Aug 31, 2021
View reviewed changes
packages/web-app-files/tests/unit/views/PrivateLink.spec.js Outdated
it.todo('should have the background image set')
it.todo('should display the page title')
it.todo('should display the logo')
it.todo('should call "getPathForFileId" method')
Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling with this description. I'd like to aim for a test driven development approach in the future. For that we'd have to write unit tests as black box tests. A test description about whether or not a certain function was called would be not allowed in such a case. Instead you'd have to describe behaviour of the component (should resolve the provided file id to a path from the backend).

@saw-jan saw-jan requested a review from kulmann August 31, 2021 07:40
@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
No Duplication information No Duplication information

kulmann
kulmann approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Awesome 👍

pascalwengerter
pascalwengerter approved these changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

LGTM, please go ahead!

@saw-jan saw-jan merged commit 250ecf5 into master Aug 31, 2021
@delete-merged-branch delete-merged-branch bot deleted the privateLink-view-tests branch August 31, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kulmann kulmann kulmann approved these changes

@pascalwengerter pascalwengerter pascalwengerter approved these changes

@dpakach dpakach Awaiting requested review from dpakach

@fschade fschade Awaiting requested review from fschade

@kiranparajuli589 kiranparajuli589 Awaiting requested review from kiranparajuli589

Assignees

@saw-jan saw-jan

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saw-jan @kulmann @pascalwengerter @kiranparajuli589