Skip to content

APS Testing [AARD-1941] #1206

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

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

APS Testing [AARD-1941] #1206

wants to merge 11 commits into from

Conversation

AlexD717
Copy link
Member

@AlexD717 AlexD717 commented Jul 10, 2025

Task

Create Tests for APS.ts

AARD-1941

Symptom

No tests for the APS system

Solution

Add tests for the APS system

Verification

All Unit tests pass


Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

AlexD717 added 5 commits July 8, 2025 14:13
APS IsSignedIn used to be returning true no matter what. This has no been fixed and APS.isSignedIn() now returns the proper result
@AlexD717 AlexD717 self-assigned this Jul 10, 2025
@AlexD717 AlexD717 requested review from a team as code owners July 10, 2025 17:23
@AlexD717 AlexD717 requested a review from rutmanz July 10, 2025 18:17
Copy link
Member

@rutmanz rutmanz left a comment

Choose a reason for hiding this comment

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

lgtm

* dev: (28 commits)
  Unused Imports Removed
  Merge Fix
  fix(merge): fix merging build issues from GH-1203
  Update fission/src/test/MirabufParser.test.ts
  New Endgame Sound & Updated Licensing Info
  fix: eslint
  fix: adding suggested changes Co-authored-by: Azalea Colburn <62953415+azaleacolburn@users.noreply.github.com>
  fix: comment position changes
  fix: eslint error
  fix: moving placement of comment
  Type & Removed Unnecessary Link
  Build Fix
  Documentation
  refactor: avoid having potentially undefined global functions
  fix: replacing empty fragment tag with null
  Cleaner Code
  Unnecessary Tests Removed
  fix: formatting
  feat: hides all touch control schemes if not supported by device
  More Advanced Joint Testing
  ...
* dev:
  fix(snyk): update version requirements
  reorder dependencies
  chore(installer): update python packages to avoid vulerabilities
  fix: clean up workflow spacing, use latest checkout and setup-node steps
  fix: use correct package.json path
  feat: add npm caching to all npm-involving workflows
Copy link
Member

@BrandonPacewic BrandonPacewic left a comment

Choose a reason for hiding this comment

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

My apologies for merging dev before suggesting these changes. Should have not merged dev till I was ready to approve.

@AlexD717 AlexD717 requested a review from BrandonPacewic July 11, 2025 22:50
Comment on lines +87 to +90
onClick={async () => {
const auth = await APS.getAuth()
auth && APS.refreshAuthToken(auth.refresh_token, true)
}}
Copy link
Member

Choose a reason for hiding this comment

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

I think the syntax is a little messed up here. I believe since auth is a promise it always is evaluated as truthly. For this reason I think an if statement is required but correct me if I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

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

auth isn't a promise here, it's APSAuth | undefined because APS.getAuth() is awaited. It's a bit unconventional to use short-circuiting in JS like this (more common in react components) but this does work

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

Successfully merging this pull request may close these issues.

5 participants