pagure

Clone

#1707 Do no convert milestones and properties if not needed and fix running the tests
Merged 7 years ago by pingou. Opened 7 years ago by pingou.
milestones  into  master

file modified
+2 -2 
@@ -947,8 +947,8 @@ 
 

          close_status=repo.close_status,
 

      )
 

      form.status.data = issue.status
 

-     form.priority.data = str(issue.priority)
 

-     form.milestone.data = str(issue.milestone)
 

+     form.priority.data = issue.priority
 

+     form.milestone.data = issue.milestone
 

      form.private.data = issue.private
 

      form.close_status.data = ''
 

      if issue.close_status:

file modified
+64 
@@ -511,6 +511,70 @@ 
 

  

      @patch('pagure.lib.git.update_git')
 

      @patch('pagure.lib.notify.send_email')
 

+     def test_view_issue_non_ascii_milestone(self, p_send_email, p_ugt):
 

+         """ Test the view_issue endpoint with non-ascii milestone. """
 

+         p_send_email.return_value = True
 

+         p_ugt.return_value = True
 

+ 

+         output = self.app.get('/foo/issue/1')
 

+         self.assertEqual(output.status_code, 404)
 

+ 

+         tests.create_projects(self.session)
 

+         tests.create_projects_git(
 

+             os.path.join(self.path), bare=True)
 

+ 

+         output = self.app.get('/test/issue/1')
 

+         self.assertEqual(output.status_code, 404)
 

+ 

+         # Create issues to play with
 

+         repo = pagure.lib.get_project(self.session, 'test')
 

+         msg = pagure.lib.new_issue(
 

+             session=self.session,
 

+             repo=repo,
 

+             title='Test issue',
 

+             content='We should work on this',
 

+             user='pingou',
 

+             ticketfolder=None
 

+         )
 

+         self.session.commit()
 

+         self.assertEqual(msg.title, 'Test issue')
 

+ 

+         # Add a non-ascii milestone to the issue but project has no milestone
 

+         issue = pagure.lib.search_issues(self.session, repo, issueid=1)
 

+         message = pagure.lib.edit_issue(
 

+             self.session,
 

+             issue=issue,
 

+             milestone=b'käpy'.decode('utf-8'),
 

+             private=False,
 

+             user='pingou',
 

+             ticketfolder=None,
 

+         )
 

+         self.assertEqual(message, 'Successfully edited issue #1')
 

+         self.session.commit()
 

+ 

+         output = self.app.get('/test/issue/1')
 

+         self.assertEqual(output.status_code, 200)
 

+         self.assertIn(
 

+             '<title>Issue #1: Test issue - test - Pagure</title>',
 

+             output.data)
 

+         self.assertNotIn(b'käpy'.decode('utf-8'), output.data)
 

+ 

+         # Add a non-ascii milestone to the project
 

+         repo = pagure.lib.get_project(self.session, 'test')
 

+         repo.milestones = {b'käpy'.decode('utf-8'): None}
 

+         self.session.add(repo)
 

+         self.session.commit()
 

+ 

+         # View the issue
 

+         output = self.app.get('/test/issue/1')
 

+         self.assertEqual(output.status_code, 200)
 

+         self.assertIn(
 

+             '<title>Issue #1: Test issue - test - Pagure</title>',
 

+             output.data)
 

+         self.assertIn(b'käpy'.decode('utf-8'), output.data.decode('utf-8'))
 

+ 

+     @patch('pagure.lib.git.update_git')
 

+     @patch('pagure.lib.notify.send_email')
 

      def test_update_issue(self, p_send_email, p_ugt):
 

          """ Test the update_issue endpoint. """
 

          p_send_email.return_value = True

file modified
+4 -1 
@@ -23,7 +23,7 @@ 
 

  

  import flask
 

  import pygit2
 

- from mock import patch
 

+ from mock import patch, MagicMock
 

  

  sys.path.insert(0, os.path.join(os.path.dirname(
 

      os.path.abspath(__file__)), '..'))
 
@@ -53,6 +53,7 @@ 
 

  

          self.app = pagure.APP.test_client()
 

  

+     @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
 

      def test_new_user(self):
 

          """ Test the new_user endpoint. """
 

  
@@ -329,6 +330,7 @@ 
 

          self.assertIn(
 

              'Email confirmed, account activated', output.data)
 

  

+     @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
 

      def test_lost_password(self):
 

          """ Test the lost_password endpoint. """
 

  
@@ -381,6 +383,7 @@ 
 

              'check your spam folder? Otherwise, try again after some time.',
 

              output.data)
 

  

+     @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
 

      def test_reset_password(self):
 

          """ Test the reset_password endpoint. """

file modified
+13 -4 
@@ -16,7 +16,7 @@ 
 

  import sys
 

  import os
 

  

- from mock import patch
 

+ from mock import patch, MagicMock
 

  

  sys.path.insert(0, os.path.join(os.path.dirname(
 

      os.path.abspath(__file__)), '..'))
 
@@ -169,8 +169,11 @@ 
 

          out = pagure.lib.notify._get_emails_for_obj(iss)
 

          self.assertEqual(out, exp)
 

  

-     def test_get_emails_for_obj_pr(self):
 

+     @patch('pagure.lib.notify.smtplib.SMTP')

Just so you know, if you don't need access to the mock object you can do

@patch('pagure.lib.notify.smtplib.SMTP', Mock(return_value=MagicMock()))
def test_get_emails_for_obj_pr(self):

Oh, I see you already know this from above :smile:

 
+     def test_get_emails_for_obj_pr(self, mock_smtp):
 

          """ Test the _get_emails_for_obj method from pagure.lib.notify. """
 

+         mock_smtp.return_value = MagicMock()
 

+ 

          tests.create_projects(self.session)
 

  

          # Create the project ns/test
 
@@ -246,8 +249,11 @@ 
 

          out = pagure.lib.notify._get_emails_for_obj(req)
 

          self.assertEqual(out, exp)
 

  

-     def test_get_emails_for_obj_pr_watching_project(self):
 

+     @patch('pagure.lib.notify.smtplib.SMTP')
 

+     def test_get_emails_for_obj_pr_watching_project(self, mock_smtp):
 

          """ Test the _get_emails_for_obj method from pagure.lib.notify. """
 

+         mock_smtp.return_value = MagicMock()
 

+ 

          tests.create_projects(self.session)
 

  

          # Create the project ns/test
 
@@ -324,8 +330,11 @@ 
 

          out = pagure.lib.notify._get_emails_for_obj(req)
 

          self.assertEqual(out, exp)
 

  

-     def test_send_email(self):
 

+     @patch('pagure.lib.notify.smtplib.SMTP')
 

+     def test_send_email(self, mock_smtp):
 

          """ Test the notify_new_comment method from pagure.lib.notify. """
 

+         mock_smtp.return_value = MagicMock()
 

+ 

          email = pagure.lib.notify.send_email(
 

              'Email content',
 

              'Email “Subject“',

The two commits of that PR can be considered as single units.
The first one stop converting the milestones and properties to string while it's not needed
and the second fix running the unit-tests without a local smtp server.

I should add unit-tests for the first of these commits, give me a few

2 new commits added

  • Small formatting fix (indentation and empty line)
  • Add unit-tests checking the behavior of view_issue when non-ascii milestone
7 years ago

Can this please be broken into two tests?

Just so you know, if you don't need access to the mock object you can do

@patch('pagure.lib.notify.smtplib.SMTP', Mock(return_value=MagicMock()))
def test_get_emails_for_obj_pr(self):

Oh, I see you already know this from above :smile:

Is there an issue associated with this?

For the first commit yes: https://pagure.io/pagure/issue/1699 for the others no tickets

Does it really matter? I thought what matter is that there is a test

Yes, if you want it to be maintainable and inviting for others to use. This function is ~150 lines and is titled test_view_issue. It's hard to review and difficult to adjust. I have no idea what cases it covers, and will have to read it all to find out, keeping in mind that often state is shared between sections.

There should be a TestCase class for each scenario (that is, this entire function should likely be a class). Each function should cover a single aspect of that scenario.

rebased
7 years ago

rebased
7 years ago

rebased
7 years ago

Rebased :)

Looks fine to me, merge on :thumbsup:

Thanks!

Pull-Request has been merged by pingou
7 years ago
Changes Summary 4
+2 -2
file changed
pagure/ui/issues.py
+64 -0
file changed
tests/test_pagure_flask_ui_issues.py
+4 -1
file changed
tests/test_pagure_flask_ui_login.py
+13 -4
file changed
tests/test_pagure_lib_notify.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.