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

Add make devcheck for developers #593

Closed
wants to merge 2 commits into from
Closed

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 15, 2017

Ticket 6604 makes pylint and jsl optional dependencies. The change
is controversal, because some developers prefer that pylint and jsl
should be required unless explicitly disabled.

make patchcheck is my answer to address the concerns. It's a superior
solution to make lint as pre-commit check. It combines several
additional checks under a single, easy rememberable and convenient make
target:

  • build all
  • acilint, apiclient, jslint, polint
  • make check
  • pylint under Python 2 and 3
  • subset of unit test suite

https://fedorahosted.org/freeipa/ticket/6604

Depends on

@tiran tiran force-pushed the patchcheck branch 3 times, most recently from 2bf52da to 561de31 Compare March 15, 2017 18:48
@tiran tiran changed the title WIP: Add make patchcheck for developers Add make patchcheck for developers Mar 15, 2017
ipatests/util.py Outdated
>>> expected = [u'Hello', dict(world=u'how are you?')]
>>> got = [u'Hello', dict(world='how are you?')]
>>> expected = [u'Hello', dict(world=1)]
>>> got = [u'Hello', dict(world=1.0)]
Copy link
Member Author

Choose a reason for hiding this comment

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

The old test scenario does no longer work on Python 3 since u'how are you' and 'how are you' have identical type. u'how are you' != b'how are you'. It works with int / float on all Python versions.

ipatests/util.py Outdated
expected = u'how are you?'
got = 'how are you?'
path = (0, 'world')
type(expected) = <... 'int'>
Copy link
Member Author

Choose a reason for hiding this comment

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

Python 2 has <type 'int'> while Python 3 uses <class 'int'>.

Makefile.am Outdated
@@ -152,6 +152,35 @@ JSLINT_TARGET = jslint
endif WITH_JSLINT
lint: acilint apilint $(POLINT_TARGET) $(PYLINT_TARGET) $(JSLINT_TARGET)

.PHONY: patchcheck
patchcheck: all
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a major nitpick but could it rather be called develcheck? The two ch in close succession don't quite roll of my keyboard, if that's not an intention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Python uses patchcheck, so I'm used to it. But I don't mind to switch to devcheck, develcheck, commitcheck or similar.

By the way, make has tab completion :)

Use the tab, Luke!

Copy link
Contributor

Choose a reason for hiding this comment

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

devcheck seems fine to me :)

@tiran tiran changed the title Add make patchcheck for developers Add make devcheck for developers Mar 30, 2017
@tiran tiran force-pushed the patchcheck branch 2 times, most recently from 5ef4045 to f41cdac Compare March 31, 2017 07:02
@stlaz stlaz self-assigned this Mar 31, 2017
@stlaz
Copy link
Contributor

stlaz commented Mar 31, 2017

The changes to Makefile and configure.ac are just fine. I understand that changes in the ipapython/session_storage.py are done elsewhere so once that is pushed, we'll need a rebase.
I don't see the explanation why we're disabling the test in ipatests/test_ipapython/test_session_storage.py , that might need a different commit?

@tiran
Copy link
Member Author

tiran commented Mar 31, 2017

test_session_storage is not a unit test or functional test. It is an integration test that depends on a valid Kerberos configuration and session. Do you prefer a separate PR?

@stlaz
Copy link
Contributor

stlaz commented Mar 31, 2017

Whichever is ok with you, I don't mind if it's in the same PR if it is related to the same ticket.

@tiran
Copy link
Member Author

tiran commented Mar 31, 2017

I split the changes to session storage tests into a separate commit. The other commit is in #670

@stlaz
Copy link
Contributor

stlaz commented Mar 31, 2017

Thanks, ACK.

@stlaz stlaz added the ack Pull Request approved, can be merged label Mar 31, 2017
@MartinBasti
Copy link
Contributor

@stlaz why is this ACKed when it depends on #670 ?

@stlaz
Copy link
Contributor

stlaz commented Mar 31, 2017

@MartinBasti #670 was ACKed already and the commit was originally a part of this.

@MartinBasti
Copy link
Contributor

Ah right the description hasn't been updated

@MartinBasti MartinBasti removed their assignment Mar 31, 2017
@MartinBasti
Copy link
Contributor

Needs rebase

Ticket 6604 makes pylint and jsl optional dependencies. The change
is controversal, because some developers prefer that pylint and jsl
should be required unless explicitly disabled.

`make devcheck` is my answer to address the concerns. It's a superior
solution to `make lint` as pre-commit check. It combines several
additional checks under a single, easy rememberable and convenient make
target:

* build all
* acilint, apiclient, jslint, polint
* make check
* pylint under Python 2 and 3
* subset of unit test suite

https://fedorahosted.org/freeipa/ticket/6604

Signed-off-by: Christian Heimes <cheimes@redhat.com>
The test class depends on a working Kerberos configuration and session.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@MartinBasti
Copy link
Contributor

master:

  • e357133 Add make devcheck for developers
  • 6c092c2 Skip test_session_storage in ipaclient unittest mode

ipa-4-5:

  • 89ab24f Add make devcheck for developers
  • c80adf6 Skip test_session_storage in ipaclient unittest mode

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants