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

Patch for step # #43

Open

GayLaurent wants to merge 33 commits into ArcaniteSolutions:master from GayLaurent:master

GayLaurent commented Oct 21, 2020

Delivery corresponding to step 1 of the service provision on the migration to Python3 / Django3

GayLaurent added 9 commits

October 14, 2020 16:28


          correct pip dependancy

3858c5d


          Merge branch 'master' of github.com:GayLaurent/truffe2

6a88a1b


          add tests script with code coverage

548eb86


          add tests for main, logistics, users, vehicles

d7d29bd


          correct tests logistics

04771ab


          append new regression tests

e763972


          append new no-regression tests

8d6f4d2


          complete README.md with option of script

7b89f24


          add tests for GenericModelWithLines

c7e858b

roosemberth suggested changes

View reviewed changes

Contributor

roosemberth left a comment

Great work! Thanks for your contribution! I've made some small comments.

tools/install_venv.sh Outdated

+              set -e
+              set +x
+              project_dir=$(dirname $(dirname $(readlink -f "$0")))

Contributor

roosemberth Oct 22, 2020

Suggested change

      
            project_dir=$(dirname $(dirname $(readlink -f "$0")))
          
            project_dir="$(dirname $(dirname $(readlink -f "$0")))"

Author

GayLaurent Oct 23, 2020

Normaly, good developper don't use space in working directory ;)
But, it's a good idea to protect about this bad using

Contributor

roosemberth Oct 24, 2020

Normaly, good developper don't use space in working directory ;)

Spaces are a valid POSIX path component. There's no reason to make any assumptions whatsoever about how the project may be deployed or developed.

There's no way to forsee how code will evolve, and this is precisely the kind to bugs that may have disastrous consequences on the user's machine.

tools/install_venv.sh Outdated

+              cp $project_dir/tools/settingsLocal.py.test $project_dir/truffe2/app/settingsLocal.py
+              (
+              	cd $project_dir/truffe2

Contributor

roosemberth Oct 22, 2020

Suggested change

      
            	cd $project_dir/truffe2
          
            	cd "$project_dir/truffe2"

Author

GayLaurent Oct 23, 2020

Normaly, good developper don't use space in working directory ;)
But, it's a good idea to protect about this bad using

tools/install_venv.sh Outdated

+              fi
+              rm -rf $project_dir/truffe2/db.sqlite3
+              cp $project_dir/tools/settingsLocal.py.test $project_dir/truffe2/app/settingsLocal.py

Contributor

roosemberth Oct 22, 2020

Suggested change

      
            cp $project_dir/tools/settingsLocal.py.test $project_dir/truffe2/app/settingsLocal.py
          
            cp "$project_dir/tools/settingsLocal.py.test" "$project_dir/truffe2/app/settingsLocal.py"

Author

GayLaurent Oct 23, 2020

Normaly, good developper don't use space in working directory ;)
But, it's a good idea to protect about this bad using

tools/install_venv.sh Outdated

+              	. $project_dir/venv/bin/activate
+              fi
+              rm -rf $project_dir/truffe2/db.sqlite3

Contributor

roosemberth Oct 22, 2020

Suggested change

      
            rm -rf $project_dir/truffe2/db.sqlite3
          
            rm -rf "$project_dir/truffe2/db.sqlite3"

Author

GayLaurent Oct 23, 2020

Normaly, good developper don't use space in working directory ;)
But, it's a good idea to protect about this bad using

tools/run_tests.sh

+              set +e
+              set +x
+              project_dir=$(dirname $(dirname $(readlink -f "$0")))

Contributor

roosemberth Oct 22, 2020

Suggested change

      
            project_dir=$(dirname $(dirname $(readlink -f "$0")))
          
            project_dir="$(dirname $(dirname $(readlink -f "$0")))"

Author

GayLaurent Oct 23, 2020

Normaly, good developper don't use space in working directory ;)
But, it's a good idea to protect about this bad using

truffe2/main/test_data.py Outdated

		SubventionFile(id=1, uploader_id=1, file='uploads/files/logo_testing.png').save()


		def initial_data():

Contributor

roosemberth Oct 22, 2020

Please add a description of what this test does.

Author

GayLaurent Oct 23, 2020

It's not a test, it's an initalisation of data function for test

Contributor

roosemberth Oct 24, 2020

Yes, sorry I meant function, my bad. In particular it would be a good idea to document what its representation invariants are. (e.g. This function initialize djago's components so that creating a user is possible).

For maintainability it's better to have functions with a restricted responsibilities and not something as generic as initialize everything possible to the moment. Specially since this may lead developers to add logic to try to configure things that the function has not enough information to do, which always ends up being a source of bugs.

Contributor

roosemberth Oct 24, 2020

It could also be argued that this function is doing too much. i.e. why would the function testing user management need to have access to configured logistics or accounting modules.

This argument is moot if the code requires this (though in such case, these kind of test function/helpers should contain comments to help newcomers understand the debts and limitations of the existing code).

truffe2/users/tests.py

+                  def test_myunit_pdf(self):
+                      self.call_check_pdf('/users/myunit/pdf/')
+                  def test_ldap_search(self):

Contributor

roosemberth Oct 22, 2020

What's the purpose of this test ?
The names of the test gives the impression it effectively verifies that a search is performed in the ldap. Whereas it is only testing an error condition. Regardless on whether such error condition is relevavant, this test should be renamed.

Author

GayLaurent Oct 23, 2020

I use generic name of test form different URL to test.

Contributor

roosemberth Oct 24, 2020

This seems to be another instance of #43 (comment), let's continue the discussions there.

truffe2/main/test_tools.py Outdated

+                      TestCase.setUp(self)
+                      initial_data()
+                      class_name = self.__class__.__name__
+                      if 'WithLogin' in class_name:

Contributor

roosemberth Oct 22, 2020

Please use constructor arguments or class attributes instead of magic class names; specially if they are not documented.

Author

GayLaurent Oct 23, 2020

Not possible in the constructor of unitcase class

Contributor

roosemberth Oct 24, 2020

Could you please develop? Can't we just do something like this?

class A:
    def __init__(self):
        pass

class B:
    def __init__(self):
        pass

class C(A, B):
    def __init__(self, *args, **kwargs):
        self.with_client = False
        if with_client in kwargs:
            self.with_client = kwargs['with_client']
            del kwargs['with_client']
        super(A, self).__init__(*args, **kwargs)

Otherwise, how about setting the attributes in the derived class' setUp method and adding a note in the class documentation?

Author

GayLaurent Nov 12, 2020

Change algorithme to select test user in "Step2"

truffe2/main/tests.py

+                  def test_link_base(self):
+                      self.call_check_html('/link/base')
+                  def test_last_100_logging_entries(self):

Contributor

roosemberth Oct 22, 2020

The name of this test is misleading, please rename it to reflect what it's actually testing.

Author

GayLaurent Oct 23, 2020

I use generic name of test form different URL to test.

Contributor

roosemberth Oct 24, 2020

I use generic name of test form different URL to test.

I'm sorry, I do not understand what you mean. Could you please rephrase?

My comment concerns the fact that this verifies an exception is risen when a URL is accessed. I fail to understand the value of testing such exception is raised.

If you want to lay work for a future test, I'd recommend rather doing an issue or testing something smaller (but concrete) and adding a note on how that test should be extended.

As-is, I have the impression the test is trying to test something that's currently not testable, completely bypassing the test altoguether (or even worse, testing that the intended functionnality fails!). A developer that sees the test test_last_100_logging_entries pass is likely to assume that the last 100 requests were verified to have been correctly recorded.

truffe2/rights/tests.py Outdated

-              # Create your tests here.
+              class RightsNoLoginTest(TestCase):
+                  def test_basic(self):

Contributor

roosemberth Oct 22, 2020

I don't understand the purpose of this test, can you please add a comment?

Author

GayLaurent Nov 12, 2020

Test removed in "Step2"

CedricCook reviewed

View reviewed changes

Contributor

CedricCook left a comment

Impressive that you are picking up work on this!

tools/settingsLocal.py.test Outdated

+                  }
+              }
+              SECRET_KEY = "*=)2_8tuf5qym&6szbr%=xz)g8aw=w3z)ltc=+d)iy7v7grj#!"

Contributor

CedricCook Oct 23, 2020

The Django secret key in test can just be set to "_" or something similar. Random keys like the one proposed may make other developers think you've leaked a key :)

Author

GayLaurent Oct 23, 2020

Yes, of course, it's juste a random key in this case.
good idea to use only "_" letters

truffe2/main/test_tools.py

+                  def call_check_redirect(self, path, data={}, method='get', redirect_url=None, target_status_code=200):
+                      self.call(path, data, method, status_expected=302)
+                      self.assertRedirects(self.response, '/users/login?next=%s' % path if redirect_url is None else redirect_url, target_status_code=target_status_code)

Contributor

CedricCook Oct 23, 2020

The /users/login string here should maybe be compared directly to django.conf.settings.LOGIN_URL? Or do you want to keep it seperate on purpose?

Author

GayLaurent Oct 23, 2020

Not in this case: it's a test, so we want be sure that it's this URL to use for redirection

Member

the-glu Oct 28, 2020

I'm not sure I agree: we should test that the view redirect to the LOGIN_URL, not a specifc url.

Checking that this URL is /users/login should be another specific test.

truffe2/main/test_tools.py

+                      if status_expected != 0:
+                          self.assertEqual(self.response.status_code, status_expected, "HTTP error [%s]:%s" % (self.response.status_code, self.get_div('text-center error-box')))
+                  def call_check_redirect(self, path, data={}, method='get', redirect_url=None, target_status_code=200):

Contributor

CedricCook Oct 23, 2020

This test essentially tests whether a view is decorated with @login_required. Is it efficient to test all of theses login protected functions by checking the redirect, or could you test for the presence of the decorator?

Author

GayLaurent Oct 23, 2020

It's juste a controle that no "login_required" is missing
And i have identified that "/accounting/main/accounting/import/step/0" has no decorator.
So, it's a no-regression control: check that nobody don't remove decorator in futur

Contributor

roosemberth commented Oct 24, 2020

@GayLaurent There's no need to repeat the same answer in each discussion ^^

The reasoning for opening many discussions is that it would be easy to "just apply" the suggestions.

roosemberth suggested changes

View reviewed changes

truffe2/main/test_data.py Outdated

		SubventionFile(id=1, uploader_id=1, file='uploads/files/logo_testing.png').save()


		def initial_data():

Contributor

roosemberth Oct 24, 2020

Yes, sorry I meant function, my bad. In particular it would be a good idea to document what its representation invariants are. (e.g. This function initialize djago's components so that creating a user is possible).

For maintainability it's better to have functions with a restricted responsibilities and not something as generic as initialize everything possible to the moment. Specially since this may lead developers to add logic to try to configure things that the function has not enough information to do, which always ends up being a source of bugs.

truffe2/main/test_data.py Outdated

		SubventionFile(id=1, uploader_id=1, file='uploads/files/logo_testing.png').save()


		def initial_data():

Contributor

roosemberth Oct 24, 2020

It could also be argued that this function is doing too much. i.e. why would the function testing user management need to have access to configured logistics or accounting modules.

This argument is moot if the code requires this (though in such case, these kind of test function/helpers should contain comments to help newcomers understand the debts and limitations of the existing code).

truffe2/main/test_tools.py Outdated

+                      TestCase.setUp(self)
+                      initial_data()
+                      class_name = self.__class__.__name__
+                      if 'WithLogin' in class_name:

Contributor

roosemberth Oct 24, 2020

Could you please develop? Can't we just do something like this?

class A:
    def __init__(self):
        pass

class B:
    def __init__(self):
        pass

class C(A, B):
    def __init__(self, *args, **kwargs):
        self.with_client = False
        if with_client in kwargs:
            self.with_client = kwargs['with_client']
            del kwargs['with_client']
        super(A, self).__init__(*args, **kwargs)

Otherwise, how about setting the attributes in the derived class' setUp method and adding a note in the class documentation?

truffe2/main/tests.py

+                  def test_link_base(self):
+                      self.call_check_html('/link/base')
+                  def test_last_100_logging_entries(self):

Contributor

roosemberth Oct 24, 2020

I use generic name of test form different URL to test.

I'm sorry, I do not understand what you mean. Could you please rephrase?

My comment concerns the fact that this verifies an exception is risen when a URL is accessed. I fail to understand the value of testing such exception is raised.

If you want to lay work for a future test, I'd recommend rather doing an issue or testing something smaller (but concrete) and adding a note on how that test should be extended.

As-is, I have the impression the test is trying to test something that's currently not testable, completely bypassing the test altoguether (or even worse, testing that the intended functionnality fails!). A developer that sees the test test_last_100_logging_entries pass is likely to assume that the last 100 requests were verified to have been correctly recorded.

the-glu requested changes

View reviewed changes

tools/settingsLocal.py.test

+              DATABASES = {
+                  'default': {
+                      'ENGINE': 'django.db.backends.sqlite3',
+                      'NAME': join(dirname(dirname(abspath(__file__))), 'db.sqlite3'),

Member

the-glu Oct 28, 2020

Is it really a good idea to use a sqlite3 database since it's not what is used in production ?

I suggest using the same database to avoid potential issues

Contributor

roosemberth Oct 28, 2020

IMHO the production database sets even a higher barrier to potentially new developers. I'd rather stay with something simple and if a CI is ever implemented use the production configuration there. In any case, it should be easily replaceable (e.g. TEST_DATABASE_URL envvar). A section in the README.md about reading the tests should be added.

Member

the-glu Oct 28, 2020

Developers should use the correct database, especially dues to various small changes that can lead to misbehavior and/or missing features.

Tests are meaningless if they fail or not depending on the context. Developers should not use sqlite3.

Contributor

TeoGoddet Nov 24, 2020

It's hard to find developers motivated to code truffe ! I'm against creating supplementary barrier. And Django provide a correct abstraction of the database. To my opinion, we should instead avoid database specific code.

Contributor

TeoGoddet Dec 7, 2020

We need to decide on this @the-glu !

Contributor

roosemberth Feb 10, 2021 •

edited

Loading

I support @TeoGoddet's argument on this one.

truffe2/accounting_main/tests.py

+                  def test_accounting_import_step2(self):
+                      sess = self.session
+                      sess.update({'T2_ACCOUNTING_IMPORT_def-123-abc': {'is_valid': True, 'has_data': True, 'year':1,
+                                                                        'data': {'nop':[],

Member

the-glu Oct 28, 2020

Can you please use coding style that don't waste all spaces and tab only to +1?

I suggest using black on new files directly (and at somepoint we should use black for the whole projet)

truffe2/accounting_main/tests.py

+                      from accounting_main.models import Budget
+                      Budget(id=2, name='bad budget', unit_id=1, accounting_year_id=1, costcenter_id=1, deleted=True).save()
+                      self.call_check_html('/accounting/main/budget/deleted', data={'upk':1})
+                      self.call_check_redirect('/accounting/main/budget/deleted', method='post', data={'upk':1, 'pk':2}, redirect_url='/accounting/main/budget/')

Member

the-glu Oct 28, 2020

In general, all thoses call only check redirection. Shouldn't them also check that (in that specific case) the object is deleted ?

Contributor

TeoGoddet Dec 7, 2020

I think the scope of the migration project was "testing that the pages doens't produce error, not that they do their job correctly"
So I would consider this out of scope

truffe2/accounting_main/tests.py

+                  def test_budget_add(self):
+                      self.call_check_html('/accounting/main/budget/~/edit', data={'upk':1, 'ypk':1})
+                      self.call_check_redirect('/accounting/main/budget/~/edit', method='post',
+                                           data={'upk':1, 'ypk':1, 'tags':'', 'name':'my budget', 'costcenter':1}, redirect_url='/accounting/main/budget/2/')

Member

the-glu Oct 28, 2020

To continue preivous comment, another example: shouldn't we test for the objcet creation in the databsae ?

Contributor

TeoGoddet Dec 7, 2020

I think the scope of the migration project was "testing that the pages doens't produce error, not that they do their job correctly"
So I would consider this out of scope

truffe2/accounting_main/tests.py

+                      self.call_check_json('/accounting/main/accountingline/json')
+                  def test_accountingline_deleted(self):
+                      from accounting_main.models import AccountingLine

Member

the-glu Oct 28, 2020

top level import ?

Contributor

TeoGoddet Dec 7, 2020

What should motivate someone to choose between TopLevenl and that kind of import ? @the-glu

Contributor

roosemberth Feb 10, 2021

Top-level imports are the norm, one should really avoid in-place imports since they nuance the readability of the code, concerns and expectations: As a developer, when I open a file, I can directly see what this file depends on and make myself a great idea of what this file does. Moreover, inline imports are not necessarily executed in all codepaths of the file, which may lead to unexpected errors at runtime that could have been avoided.

truffe2/main/test_data.py

+                  ExpenseClaim(id=1, name='expense', user=user, nb_proofs=1, accounting_year_id=1, costcenter_id=1, status='6_archived').save()
+                  ExpenseClaimLine(expense_claim_id=1, label='claim', account_id=5, value=54.65, tva=0, value_ttc=54.65).save()
+                  ExpenseClaimLogging(object_id=1, when=now, who=user, what='created').save()
+                  media_path = join(dirname(dirname(__file__)), 'media')

Member

the-glu Oct 28, 2020

You should probably use the django settings value for that instead of computing it again

truffe2/main/test_tools.py Outdated

+              from django.test.client import MULTIPART_CONTENT
+              from django.core.management import call_command
+              from main.test_data import initial_data

Member

the-glu Oct 28, 2020

The import order doesn't follow the global convention in the project.

[django imports]


[project import]


[others imports]

(with double crlf)

(Yes, that non-standard, but it's better to follow existing convention for coherence :) )

Author

GayLaurent Nov 12, 2020

This convention is not standard.
Personnaly, i used to more generic (Python) or more specific (project):

[others imports]

[django imports]

[project import]

Member

the-glu Nov 12, 2020

Like I said, yes it's not standard, but projet standards are more important :)

truffe2/main/test_tools.py Outdated

+                          self.connect_to('admin')
+                      elif 'WithLog' in class_name:
+                          user_num = int(class_name[class_name.index('WithLog') + 7])
+                          self.connect_to('user%d' % user_num)

Member

the-glu Oct 28, 2020

Also as a global convention, please use format instead of % syntax.

truffe2/main/test_tools.py

+                  def call_check_redirect(self, path, data={}, method='get', redirect_url=None, target_status_code=200):
+                      self.call(path, data, method, status_expected=302)
+                      self.assertRedirects(self.response, '/users/login?next=%s' % path if redirect_url is None else redirect_url, target_status_code=target_status_code)

Member

the-glu Oct 28, 2020

I'm not sure I agree: we should test that the view redirect to the LOGIN_URL, not a specifc url.

Checking that this URL is /users/login should be another specific test.

truffe2/main/test_tools.py

+                      if status_expected != 0:
+                          self.assertEqual(self.response.status_code, status_expected, "HTTP error [%s]:%s" % (self.response.status_code, self.get_div('text-center error-box')))
+                  def call_check_redirect(self, path, data={}, method='get', redirect_url=None, target_status_code=200):

Member

the-glu Oct 28, 2020

I think the function name is missleading. It's not checking for a redirection but for a redirection to the login page.

GayLaurent and others added 2 commits

October 28, 2020 20:30


          corections after feetback of truffe2 community

59f476b


          migrate from Django 1.6 to Django 1.7

008795e

Contributor

roosemberth commented Oct 30, 2020 •

edited

Loading

@GayLaurent I think a different PR should be opened to include Django migrations. This PR should be merged before any of the other ones can proceed.

@the-glu what's blocking the merge of 59f476b?

Member

the-glu commented Oct 30, 2020

@the-glu what's blocking the merge of 59f476b?

Some of my comments are still open ? Not sure about your question. Did you used the wrong hash ?

Contributor

roosemberth commented Oct 30, 2020

Some of my comments are still open ? Not sure about your question. Did you used the wrong hash ?

Sorry, my client interface was incoherent. From the github.com's web interface what needs to be adressed is clear.

Laurent GAY added 6 commits

October 31, 2020 16:20


          migrate from Django 1.7 to Django 1.8

7ceaaa4


          migrate from Django 1.8 to Django 1.9

2d03683


          correct wsgi

071dae9


          correct list json

cb20e0a


          migrate from Django 1.9 to Django 1.10

a263a96


          migrate from Django 1.10 to Django 1.11

6278bad

GayLaurent closed this

Author

GayLaurent commented Nov 3, 2020

Cancel to submit a new pull request

TeoGoddet and others added 3 commits

November 4, 2020 11:13


          Fix missing change in url ref from . to -


          Merge pull request #1 from TeoGoddet/master

3a5b3d7

Fix missing change in url ref from . to -


          correction about migration 1.6->1.11 on production server

97c075a

Laurent GAY and others added 10 commits

November 5, 2020 16:46


          correction about tequila login

bf529cd


          Fix for guest account testing

f7a6f32


          change PDF generator and upgrade old components

e2a2a15


          migrate from Python 2.7 to Python 3.5

b100bcd


          migrate from Python 3.5 to Python 3.7

78904ce


          Library Py3 for HTML to PDF

b5812e6


          change README & deployement script for Py3

e95284c


          Merge pull request #2 from TeoGoddet/master

c786c5a

Fix guest account testing


          correct Python3 for tequila login

193f377


          correct for tequila login

97bf598

Member

llann commented Nov 23, 2020

Hey @GayLaurent that's not really the way to do it... Now it's hard to see if changes have been done. Can you please re-open it and apply changes here until it's validated and then merge them in your new branch and do a new PR. This is blocking the whole process.

Author

GayLaurent commented Nov 25, 2020

Re-open to be apply in your master branch

GayLaurent reopened this

Author

GayLaurent commented Nov 25, 2020

The main remarks you made are taken into account in the following PR.
Please validate this PR so that I can submit the rest to you.


          migrate to Django 2.2 + upgrade library + correct PDF

86f1c65

Contributor

roosemberth commented Feb 10, 2021

@GayLaurent Can you please address the discussions above and leave a comment with the hash of the commit addressing each of them?

Thanks!

Laurent GAY added 2 commits

February 11, 2021 11:51


          migrate to Django 3.1 + check Python 3.8

f77716f


          Migrate correction: map/filter using -> typecast to list + replace it…

477f940

…eritems to items in HTML templates for dict access

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet