Skip to content

Add tests for BoardsServiceProvider #1337

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

Closed
wants to merge 10 commits into from

Conversation

AlbyIanna
Copy link
Contributor

Motivation

The following tests — marked with 1) and 2) — are failing on the main branch and should succeed here.

When there is one recognized boards attached
  and there is one unrecognized devicec available on a port
    and the port with the recognized board gets selected
      and then the port with the unrecognized board gets selected
        and then the port with the recognized board gets selected again
          and the recognized board gets unplugged because of an upload in progress
            1) board should remain selected with NO port selected
            and the recognized board gets plugged in again into the same port
              2) board should remain selected with NO port selected

Change description

Other information

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@AlbyIanna AlbyIanna marked this pull request as draft August 19, 2022 09:09
Comment on lines +370 to +378
get latestValidBoardsConfig():
| RecursiveRequired<BoardsConfig.Config>
| undefined {
return this._latestValidBoardsConfig;
}

get latestBoardsConfig(): BoardsConfig.Config | undefined {
return this._latestBoardsConfig;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you use public APIs for the tests or do you think this will be used anywhere else in the app?

If the former, I propose using protected and subclass for the tests and increasing the visibility. If the latter, please explain where you plan to use it.

Comment on lines +141 to +143
subject.onAvailablePortsChanged(() => {
expect(true).to.be.ok;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test fail if the event is not received?

Base automatically changed from 180-board-selection to main August 24, 2022 10:31
@davegarthsimpson davegarthsimpson added type: enhancement Proposed improvement priority: low Resolution is a low priority status: on hold Do not proceed at this time labels Sep 8, 2022
@AlbyIanna
Copy link
Contributor Author

Closing this PR for now as the tested feature changed substantially since these tests were written.

@AlbyIanna AlbyIanna closed this Sep 27, 2022
@per1234 per1234 added conclusion: invalid Issue/PR not valid and removed status: on hold Do not proceed at this time labels Sep 27, 2022
@AlbyIanna AlbyIanna deleted the 180-board-selection-with-tests branch February 6, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: invalid Issue/PR not valid priority: low Resolution is a low priority type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants