"W. Trevor King" [off-list ref] writes:
quoted
I think it would be better to add initial tests with refactoring, and
snapshot specific tests with snapshot support, e.g.:
1/2: gitweb: Refactor If-Modified-Since handling and add tests
2/2: gitweb: Add If-Modified-Since support for snapshots
But the new tests would be for the new functionality (i.e. snapshot
support), so they wouldn't belong in the general refactoring commit.
Then you are planning to split it in a wrong way.
As I said, I do not think it matters that much for a small patch like
this, but if the plan is to make the part to create i-m-s helper function
as a standalone "refactoring" patch, then what Jakub outlined is the right
way to go about it.
In the first patch, you create i-m-s helper and update the existing code
that can use the helper, without changing anything else. Do not touch
snapshot code in this patch, if the current code does not support i-m-s in
snapshot. And in the same patch, add tests for codepaths that use i-m-s
to make sure your refactoring did not break them. In other words, if you
remove the change to gitweb/ from the first patch and apply only the
changes to the tests, the resulting new tests should pass with the current
code that has i-m-s support inline without the i-m-s helper. And if you
add back your change to gitweb/ for the refactoring, the test should still
pass.
And then in the second patch, you update the snapshot code and whatever
else that can use i-m-s helper to support i-m-s. You can add tests to
protect the new feature from future breakages in this patch.
W. Trevor King wrote:
On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote:
quoted
"W. Trevor King" [off-list ref] writes:
quoted
quoted
quoted
I think it would be better to add initial tests with refactoring, and
snapshot specific tests with snapshot support, e.g.:
1/2: gitweb: Refactor If-Modified-Since handling and add tests
2/2: gitweb: Add If-Modified-Since support for snapshots
But the new tests would be for the new functionality (i.e. snapshot
support), so they wouldn't belong in the general refactoring commit.
Then you are planning to split it in a wrong way.
... add tests for codepaths that use i-m-s to make sure your
refactoring did not break them...
Ah, I was assuming that some current tests might be checking the
current behavior, and that my new tests would be testing my new
snapshot behavior. If the old i-m-s handling also needs tests, that
should happen before any of my previously proposed patches:
1: tests for i-m-s and git_feed
2: refactor i-m-s handling
Those two can be in single commit. Tests added need only test i-m-s
using git_feed ('atom' or 'rss' action), as it is the only user,
and you touch only i-m-s handling.
3: tests for i-m-s and git_snapshot (which fail until 4)
4: add i-m-s to git_snapshot
We usually put tests together with feature. Tests before feature means
that you would need to mark them as test_expect_failure, as they would
not pass before feature is added, isn't it?
--
Jakub Narebski
Poland
On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote:
"W. Trevor King" [off-list ref] writes:
quoted
quoted
I think it would be better to add initial tests with refactoring, and
snapshot specific tests with snapshot support, e.g.:
1/2: gitweb: Refactor If-Modified-Since handling and add tests
2/2: gitweb: Add If-Modified-Since support for snapshots
But the new tests would be for the new functionality (i.e. snapshot
support), so they wouldn't belong in the general refactoring commit.
Then you are planning to split it in a wrong way.
... add tests for codepaths that use i-m-s to make sure your
refactoring did not break them...
Ah, I was assuming that some current tests might be checking the
current behavior, and that my new tests would be testing my new
snapshot behavior. If the old i-m-s handling also needs tests, that
should happen before any of my previously proposed patches:
1: tests for i-m-s and git_feed
2: refactor i-m-s handling
3: tests for i-m-s and git_snapshot (which fail until 4)
4: add i-m-s to git_snapshot
How does that sound?
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy