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 tests and separate from class validation merge methods #363

Open
wants to merge 13 commits into
base: story/load-multiple-mcaps-r1
Choose a base branch
from

Conversation

aneuwald-ctw
Copy link
Contributor

@aneuwald-ctw aneuwald-ctw commented Feb 7, 2025

User-Facing Changes
Added validation for merging MCAP files and introduced unit tests for the load-multiple-mcaps story.

Description

  • Refactored merging and validation logic into a separate file for better testability.
  • Added unit tests for mergeAsyncIterators and MultiIterableSource.
  • Fixed a typo in the Initialization interface (previously Initalization).

Checklist

  • The web version was tested and it is running ok
  • The desktop version was tested and it is running ok
  • This change is covered by unit tests
  • Files constants.ts, types.ts and *.style.ts have been checked and relevant code snippets have been relocated

@aneuwald-ctw aneuwald-ctw requested a review from luluiz February 7, 2025 16:10
@aneuwald-ctw aneuwald-ctw marked this pull request as ready for review February 11, 2025 11:55
@aneuwald-ctw
Copy link
Contributor Author

I don't understand why the MultiIterableSource.ts is considered as 0% tested. I believe that it is something related to how sonar is configured. I suggest to merge it and check when we point to merge to main if it will be considered.

@@ -36,7 +44,7 @@ export class MultiIterableSource<T extends IIterableSource, P> implements IItera
this.SourceConstructor = SourceConstructor;
}

private async loadMultipleSources(): Promise<Initalization[]> {
private async loadMultipleSources(): Promise<Initialization[]> {
const { type } = this.dataSource;

const sources: IIterableSource[] =

Choose a reason for hiding this comment

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

suggestion: This branching should be tested. Since this is a private method, I would suggest testing where its behaviour is expected. Therefore, during initialization.
Tests would look something like the following:

it("source type is file", async () => {
    dataSource = {
      type: "files",
      files: [new Blob(), new Blob()],
    };
    const multiSource = new MultiIterableSource(dataSource, mockSourceConstructor);
    await multiSource.initialize();

    expect(mockSourceConstructor).toHaveBeenCalledWith(expect.objectContaining({ type: "file" }));
  });

  it("source type is url", async () => {
    dataSource = {
      type: "urls",
      urls: ["my.url/"],
    };
    const multiSource = new MultiIterableSource(dataSource, mockSourceConstructor);
    await multiSource.initialize();

    expect(mockSourceConstructor).toHaveBeenCalledWith(expect.objectContaining({ type: "url" }));
  });

Copy link
Member

Choose a reason for hiding this comment

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

done

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
77.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants