Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #301563 +++ We need to add unittests for the fast project import, including the changes requested by Szymon as per bug 301563 comment 37 item 3.
Created attachment 162449 [details] Patch to org.eclipse.core.tests.resources This patch defines two unittests for the fast project import from snapshot data. As previously requested, I have refactored ExportSnapshotTest into org.eclipse.core.resources. Later I will also add a performance test that can be integrated into the standard performance tests for core.resources.
Created attachment 162554 [details] Patch with slight modifications I slightly updated the patch as follows: - Added Javadoc Class comments explaining the tests - Removed unnecessary dependency on base class LocalStoreTest and CoreConstants - Ranamed methods to better match the final API names (project load/save) - Hooked into AllTests Szymon, is this what you had in mind? I considered integrating the API tests into IProjectTest, but then I thought that the snapshot functionality is an independent bit of functionality and we'll likely want more tests for special cases and similar setUp / tearDown requirements go into the same test class (e.g. handling linked resources, rename-on-import, ...). So I used ProjectSnapshotTest for now, but I'll easily be swayed otherwise.
Right, I think it is better to have ProjectSnapshotTest instead of adding new methods to IProjectTest. The performance test in the patch should use PerformanceTestRunner as it's already made in perf tests added in bug 306573. I would not add new tests to WorkspacePerformanceTest, but use ProjectSnapshotPerformanceTest instead.
(In reply to comment #3) The goal behind having ProjectSnapshotPerformanceTest is different than the other performance tests. See also 306573 comment 13, and the class javadoc in ProjectSnapshotPerformanceTest.
Comment on attachment 162449 [details] Patch to org.eclipse.core.tests.resources I have committed the ProjectSnapshotTest (slightly modified version) as well as hooked into AllTests. Szymon: Would it help understanding the situation if I renamed the ProjectSnapshotPerformanceTest into e.g. "ProjectSnapshotManualPerformanceTest" ? - As mentioned before, I would like to keep this test in because it provides a facility for testing with-snapshot against without-snapshot on any real world live project easily. With this being its value, it cannot be tied into the usual performance test suite (PerformanceTestRunner). That's what we have the other performance tests for. Please let me know your thoughts such that I can commit.
(In reply to comment #5) > (From update of attachment 162449 [details]) > I have committed the ProjectSnapshotTest (slightly modified version) as well as > hooked into AllTests. > > Szymon: Would it help understanding the situation if I renamed the > ProjectSnapshotPerformanceTest into e.g. "ProjectSnapshotManualPerformanceTest" > ? - As mentioned before, I would like to keep this test in because it provides a > facility for testing with-snapshot against without-snapshot on any real world > live project easily. Right. I was confused by the name. I thought you want to tie this test to the automated perf test suite. We need to distinguish this test from other automated tests. To prevent including it into the automated test suite accidentally. We can just rename it to ProjectSnapshotManualPerformanceTest or create a separate package 'manual' for such tests. The second is what probably UI tests do. Moreover even if this test is run and has to be verified manually, we still can use PerormanceTestRunner. It would provide more perf details than just time of execution.
Good points. While refactoring into a "manual" package sounds compelling at first sight, I'm a little concerned that this might make it harder to re-use infrastructure / functionality from any existing package. Also, it might make it harder for people who don't know the structure to actually find the test. Consider the existing RefreshLocalPerformanceTest, which we used as the base. It is also manual, and it is also in the same package as the other tests related to refresh. Taking this into account, I'm more leaning towards a naming convention (e.g. "ManualTest" as postfix) to denote tests which are NOT meant to run in the AutomatedTests suite, but keeping the package association related to functionality. If you agree, I'll commit it as "ProjectSnapshotPerfManualTest" so the name doesn't get way too long. Once committed, I'd create a separate bug for enhancing it (as well as the related RefreshLocalPerformanceTest) to build on top of the Perftest infrastructure. Sounds good?
(In reply to comment #7) Sounds good :)
Committed the manual performance test as ProjectSnapshotPerfManualTest I think that together with the fix for bug 307985, the performance tests in bug 306573 and the rename-on-import test on bug 305718 we can consider this FIXED > N20100407. Thanks Francis for this contribution.
Please raise a bug for enhancing manual perf tests as described in comment 7.
(In reply to comment #10) > Please raise a bug for enhancing manual perf tests as described in comment 7. Huh? - For snapshot, all changes from comment 7 have been incorporated and committed already. Are you talking about other manual tests? The only other one I am aware of is RefreshLocalPerformanceTest. Sure I can rename that one. I don't know whether others exist, or if you want anything else to be documented or done... in short, I'm having a hard time raising a bug for you since I don't exactly know what you want to accomplish.
(In reply to comment #11) See comment 7: "Once committed, I'd create a separate bug for enhancing it (as well as the related RefreshLocalPerformanceTest) to build on top of the Perftest infrastructure. Sounds good?"
Uh-oh, I missed this... too much going on in parallel. Created bug 309216 - Use perftest infrastructure in ProjectSnapshotPerfManualTest
