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

#200 - Fixing test_archive_license_requests_feature and test_unarchive_license_requests_feature #457

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BanulaKumarage
Copy link
Contributor

@BanulaKumarage BanulaKumarage commented Apr 1, 2023

  • Logged in with the user setup in secrets.py
  • Add client sessionid to selenium driver
  • Mocked the app.utils.checkPermission to return true

When the app.utils.checkPermission returns true, context_dict['authorized'] becomes true and that cause archive/ unarchive buttons visible in the UI.

image

…chive_license_requests_feature

Mocked utils.checkPermissionto retrurn true
Copy link
Collaborator

@rtgdk rtgdk left a comment

Choose a reason for hiding this comment

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

Overall LGTM, left 1 comment to address

src/app/tests.py Outdated
self.user = authenticate(username=TEST_LOGIN_INFO["login"],
password=TEST_LOGIN_INFO["password"])
login = self.client.login(username=TEST_LOGIN_INFO["login"],
password=TEST_LOGIN_INFO["password"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's create a TestUtil for common code to setup github login.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A separate class or a function inside the ArchiveLicenseRequestsViewsTestCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rtgdk Should I include this in utils.py or a separate file named testUtils.py

Copy link
Collaborator

@rtgdk rtgdk Apr 16, 2023

Choose a reason for hiding this comment

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

Let's have test_utils.py or keep the util inside tests.py itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rtgdk done

@BanulaKumarage BanulaKumarage requested a review from rtgdk April 16, 2023 05:09
src/app/tests.py Outdated
@@ -21,6 +22,7 @@

from app.models import UserID
from app.models import LicenseRequest, LicenseNamespace
import app.utils
Copy link

Choose a reason for hiding this comment

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

5% of developers fix this issue

F401: 'app.utils' imported but unused


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -31,6 +33,32 @@
def getExamplePath(filename):
return os.path.join(settings.EXAMPLES_DIR, filename)

class TestUtil(TestCase):
Copy link

Choose a reason for hiding this comment

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

1% of developers fix this issue

E302: expected 2 blank lines, found 1


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

src/app/tests.py Outdated
login = self.client.login(username=TEST_LOGIN_INFO["login"],
password=TEST_LOGIN_INFO["password"])
return login

Copy link

Choose a reason for hiding this comment

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

1% of developers fix this issue

W293: blank line contains whitespace


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

with patch('app.utils.checkPermission') as mock_checkPermission:
mock_checkPermission.return_value = True
driver.get(self.live_server_url+'/app/license_requests/')
driver.add_cookie({'name': 'sessionid', 'value': cookie.value, 'secure': False, 'path': '/'})
Copy link

Choose a reason for hiding this comment

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

1% of developers fix this issue

E501: line too long (105 > 79 characters)

❗❗ 11 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
src/app/tests.py 1147
src/app/tests.py 1151
src/app/tests.py 1152
src/app/tests.py 1153
src/app/tests.py 1155
src/app/tests.py 1169
src/app/tests.py 1172
src/app/tests.py 1176
src/app/tests.py 1177
src/app/tests.py 1178

Showing 10 of 11 findings. Visit the Lift Web Console to see all.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sonatype-lift ignoreall

Copy link

Choose a reason for hiding this comment

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

The ignoreall command is active on this PR, all the existing Lift issues are ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants