Re: [PATCH openbmc-test-automation 00/60] Pull Manjunath's automation repository

["Manjunath A Kumatagi" <mkumatag@in.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5331 bytes --]


These code changes are done long back in my private repository, now part of
mirroring process into openbmc org - we are seeing all of these big list of
commits again in the PR. However all points mention below are very valid
observation and I'll make sure to follow those guidelines going forward.

Thanks,
Manjunath.


From:	Cyril Bur <cyrilbur@gmail.com>
To:	OpenBMC Patches <openbmc-patches@stwcx.xyz>
Cc:	openbmc@lists.ozlabs.org
Date:	01/12/2016 05:41 AM
Subject:	Re: [PATCH openbmc-test-automation 00/60] Pull Manjunath's
            automation repository
Sent by:	"openbmc" <openbmc-bounces
            +mkumatag=in.ibm.com@lists.ozlabs.org>



On Mon, 11 Jan 2016 10:49:26 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> Pull Manjunath's repository:
https://github.com/mkumatag/openbmc-automation
> into openbmc.
>

Ummmm what happened here? It looks like the entire series is sent twice?

I've had an initial skim read and it looks to me like you can squash A LOT
of
those patches together.

The commits don't need to be a story of how you got from no code to the
this point in time. Each commit should add a feature and associated
documentation. It looks like most of patches 0-6 can be one commit, same
for
some subsequent patches.

Heres an example of some issues I've noticed:

In patch 2 you add tests/test_ssh.robot but then in patch 7 you delete
tests/test_ssh.robot. There are several examples of a patch adding a
significant chunk of work and then a later patch removing this chunk of
work, if
after the series the chunk of work won't be in the repo then it shouldn't
have
appeared in the series at all.

Patch 26 "Fix errors" should be squashed into the broken commit. Patch 22
looks
that way too and Patch 17. The reason we do this is that way when something
does go wrong and something doesn't work it makes it much easier to find
the
broken patch, you should never submit a patches that is broken even if it
is
fixed directly after this breaks the assumption that every commit we make
is
good and therefore when chasing a bug if a broken patch is found, is it
likely
to be that bug, much easier to automate.

Your patch 28 that "Adds one more ssl test" should be squashed into the
patch
adding the ssl tests, add them all once for this series. You can always
more
work on more later but this series you have one patch adding ssl tests,
have
all the ssl tests you're adding in that patch.

Hopefully that makes sense, please ask if I've just rambled!

Cyril

> https://github.com/openbmc/openbmc-test-automation/pull/1
>
> Chris Austen (16):
>   Added DIMM specific inventory tests
>   base add for sensor tests
>   fixed review comments
>   created loops for tests
>   Add initial rest test cases     Support to test Null properties
>   Added full sensor suite
>   Update README.md
>   Handle new rest client from Brad
>   Added DIMM specific inventory tests
>   base add for sensor tests
>   fixed review comments
>   created loops for tests
>   Add initial rest test cases     Support to test Null properties
>   Added full sensor suite
>   Update README.md
>   Handle new rest client from Brad
>
> Manjunath A Kumatagi (34):
>   Added git ignore file
>   Use argumnet file with pyrobot and added instructions to READEME file.
>   Update README for better html display
>   Added Inventory list testcase
>   fixed errors in tox and generate_argumentfile.sh script
>   Moved Read Attribute and Read Properties to rest_client
>   fix review comment
>   Added SSL tests
>   Ran the tests and fixed errors in SSL tests
>   Modified the doc
>   Added firmware version validation test
>   update readme file with tox version
>   Fix errors
>   Fix Errors in test_obmcrest.robot and removed the empty testcases.
>   Added one more testcase to test non-ssl connection to port 443
>   OpenBMC REST Testing
>   updated the REST API for firmware version
>   Added git ignore file
>   Use argumnet file with pyrobot and added instructions to READEME file.
>   Update README for better html display
>   Added Inventory list testcase
>   fixed errors in tox and generate_argumentfile.sh script
>   Moved Read Attribute and Read Properties to rest_client
>   fix review comment
>   Added SSL tests
>   Ran the tests and fixed errors in SSL tests
>   Modified the doc
>   Added firmware version validation test
>   update readme file with tox version
>   Fix errors
>   Fix Errors in test_obmcrest.robot and removed the empty testcases.
>   Added one more testcase to test non-ssl connection to port 443
>   OpenBMC REST Testing
>   updated the REST API for firmware version
>
> Manjunath Kumatagi (2):
>   first commit
>   first commit
>
> manjunath (6):
>   added files
>   added log statement
>   added testcase for etc passwd
>   added files
>   added log statement
>   added testcase for etc passwd
>
> mkumatag (2):
>   Update README.md
>   Update README.md
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc



[-- Attachment #1.2: Type: text/html, Size: 7416 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]