Add a mapping from revision to branches while using only one exp show call #3980
Conversation
| ) | ||
| expect(experimentsByCommit.get('branchB')).toBeUndefined() | ||
| }) | ||
|
|
||
| it('should add data from git to commits if git log output is provided', () => { |
There was a problem hiding this comment.
This should be a test to recreate but after a call to update not after collectExperiments. I was still at fixing failing tests.
There was a problem hiding this comment.
covered by the integration tests/it('should return the expected rows when given the base fixture'
| ...out, | ||
| branch, | ||
| commit, | ||
| description: formatCommitMessage(commit.message) |
There was a problem hiding this comment.
[Q] Can I move this upstream into the collect call?
| starred: false | ||
| } | ||
| ] | ||
| const getIds = (rows: Commit[]) => |
There was a problem hiding this comment.
[F] I always run into/get annoyed by this test. It was previously testing more than it needed to. It is not testing only the functionality that it needs to.
|
|
||
| return this.notifyChanged(data) | ||
| return this.notifyChanged({ currentBranch, expShow, gitLog, order }) |
There was a problem hiding this comment.
[F] By doing the least amount of processing possible here we can update the test fixture(s) with raw data and make sure that everything still works.
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| ]) | ||
| }) | ||
|
|
||
| it('should handle duplicate commits being returned in the data', () => { |
There was a problem hiding this comment.
[F] This can no longer happen
| @@ -449,20 +454,6 @@ export class ExperimentsModel extends ModelWithPersistence { | |||
| return this.currentSorts.findIndex(({ path }) => path === pathToRemove) | |||
| } | |||
|
|
|||
| private getCommits() { | |||
There was a problem hiding this comment.
[F] This is no longer required because we're not holding duplicates
| @@ -30,6 +30,30 @@ export const experimentsWithCommits: Experiment[] = [ | |||
| }, | |||
| params: { 'params.yaml': { dropout: 0.122, epochs: 5 } } | |||
| }, | |||
| { | |||
| branch: 'main', | |||
| id: 'fe2919b', | |||
There was a problem hiding this comment.
[F] There are changes to tests and fixtures because I added two more commits to the base fixture.
| @@ -169,13 +168,12 @@ export class Experiments extends BaseRepository<TableData> { | |||
| return this.data.managedUpdate() | |||
| } | |||
|
|
|||
| public async setState(data: ExpShowOutput) { | |||
| public async setState({ expShow, gitLog, rowOrder }: ExperimentsOutput) { | |||
There was a problem hiding this comment.
[I] getNumCommits should come out of WebviewMessages and called in ExperimentsData as well.
There was a problem hiding this comment.
I've got an item on the board to follow up and do this but will do it next.
| @@ -596,7 +575,7 @@ export class Experiments extends BaseRepository<TableData> { | |||
| } | |||
|
|
|||
| return await pickExperiment( | |||
| this.experiments.getUniqueList(), | |||
| this.experiments.getCombinedList(), | |||
There was a problem hiding this comment.
[F] This name had to change as the old name was misleading (data is no longer duplicated)
| dvcLiveOnly: boolean, | ||
| commitsOutput: string | undefined | ||
| expShow: ExpShowOutput, | ||
| gitLog: string, |
There was a problem hiding this comment.
[F] To me it makes more sense to put the gitLog output before dvcLiveOnly because it is more important for the data.
|
|
||
| commits.sort((a, b) => (b.Created || '').localeCompare(a.Created || '')) |
There was a problem hiding this comment.
[F] Doing this means we get commits sorted in chronological order in the tree.
| expShow = expShowFixture, | ||
| dvcRoot = dvcDemoPath, | ||
| gitLog = gitLogFixture, | ||
| rowOrder = rowOrderFixture |
There was a problem hiding this comment.
[I] Used VS Code's Refactor... Convert parameters to destructured object. The number of optional parameters was out of control. Unfortunately, that increases the diff a bit.
| plotsDiff: PlotsOutput | undefined = undefined, | ||
| expShow = expShowFixtureWithoutErrors | ||
| ) => { | ||
| export const buildPlots = async ({ |
There was a problem hiding this comment.
[F] Did the same refactoring for this as it also has too many inputs now.
| @@ -36,3 +36,4 @@ const MockComponentWithChildren: React.FC<PropsWithChildren> = ({ | |||
| export const VSCodePanels = MockComponentWithChildren | |||
| export const VSCodePanelTab = MockComponentWithChildren | |||
| export const VSCodePanelView = MockComponentWithChildren | |||
| export const VSCodeTag = MockComponentWithChildren | |||
There was a problem hiding this comment.
[F] Adding tags to the rows fixture meant that this was triggered and broke all of the jest tests.
| public async getCommitMessages( | ||
| cwd: string, | ||
| revision: string, | ||
| revisions: string |
There was a problem hiding this comment.
Minor: Should the revisions be a number instead of a string then we change the type to string in the getOptions call below?
|
Code Climate has analyzed commit 9002047 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 98.7% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.0% change). View more on Code Climate. |
For #1966
Screen.Recording.2023-05-25.at.2.10.59.PM.mov
Named revision might still change (only one is being returned by dvc), but no more shift of commits between branches.