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

[RunAllTests] Test coverage workflow run all tests on Origin #5519

Commits on Aug 20, 2024

  1. Configuration menu
    Copy the full SHA
    e049c8e View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    1c6c9f7 View commit details
    Browse the repository at this point in the history

Commits on Aug 24, 2024

  1. Fixes part of oppia#4938: Use TranslationController as the source of …

    …truth for the audio language setting (oppia#5487)
    
    ## Explanation
    Fixes part of oppia#4938
    
    This primarily updates ``ProfileManagementController`` to use
    ``TranslationController`` as the source of truth for audio language
    (rather than using the audio language property stored within the
    ``Profile`` proto). This has some noteworthy advantages:
    - It allows for proper translation setting fallback behaviors to be
    enabled for the audio language setting without needing to migrate UI
    code over to ``TranslationController``.
    - It isolates changes to the domain layer (with one exception: reading
    the audio language setting now uses a new getter in
    ``ProfileManagementController`` rather than reading the profile
    directly).
    
    Some peripheral changes were also needed as part of this:
    - A bunch of tests needed to be disabled in Gradle now that different
    options UIs (including for reading text) may depend on
    ``TranslationController`` (which is only fully configured in Bazel
    builds).
    - The ``Profile`` proto was updated to remove its audio language
    setting. This means that existing users will revert back to whichever
    default ``TranslationController`` decides for them (most likely English,
    but it depends on several factors). This is considered a reasonable
    regression since most users are unlikely to depend on the automatic
    audio language setting, and the app is currently in a beta state so
    regressions like this should be expected.
    - French and Chinese were removed from the list of ``AudioLanguage``s
    since they are not currently supported by the Oppia Android app (per
    ``OppiaLanguage`` which, plus the configured supported language
    textproto files, determine for which languages the app is guaranteeing
    support).
    - ``ProfileManagementControllerTest`` was updated to have much more
    thorough testing around audio language.
    - ``TranslationController`` was updated to use a
    ``PersistentCacheStore`` backing for audio and written translation
    languages (in addition to app language which was already supported).
    - As part of the previous change,
    ``TranslationController.updateAppLanguage()`` and its related tests were
    updated to verify the _previous_ language is returned, not the current
    (for consistency with voiceover and written translations).
    
    Long-term, ``AudioLanguage`` should be removed (along with its
    corresponding functionality in ``ProfileManagementController``) in favor
    of using ``TranslationController`` and ``OppiaLocale`` as the bases for
    managing language functionality in the UI layer.
    
    Note that the Gradle version of the app will have increased degraded
    functionality in the options menu due to no supported language
    configuration being included in the build for that app (see the
    corresponding comment thread in this PR for more context). This is
    considered a reasonable medium-term gap as developers ought to be using
    the Bazel build of the app, anyway, as the Gradle version has
    significant functional limitations for all aspects of language selection
    and management (due to the lack of language configuration).
    
    ## Essential Checklist
    - [x] The PR title and explanation each start with "Fix #bugnum: " (If
    this PR fixes part of an issue, prefix the title with "Fix part of
    #bugnum: ...".)
    - [x] Any changes to
    [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
    files have their rationale included in the PR explanation.
    - [x] The PR follows the [style
    guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
    - [x] The PR does not contain any unnecessary code changes from Android
    Studio
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
    - [x] The PR is made from a branch that's **not** called "develop" and
    is up-to-date with "develop".
    - [x] The PR is **assigned** to the appropriate reviewers
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).
    
    ## For UI-specific PRs only
    
    Options screen (without changes):
    
    
    ![image](https://github.com/user-attachments/assets/065caa9f-5815-42a5-98ec-278186fa8dcd)
    
    Options screen (with changes):
    
    
    ![image](https://github.com/user-attachments/assets/f4ece283-bf0e-4490-a1bb-57e77bb7a834)
    
    I also verified setting a profile audio setting (Portuguese) on a
    ``develop`` branch build and then upgrading to a build from this branch.
    There are no crashes or stability issues, and (per my device setup) the
    audio language does revert back to English as expected.
    
    I also verified that, like before, the audio setting persists across app
    instances when changed and is distinct between multiple profiles.
    BenHenning authored and adhiamboperes committed Aug 24, 2024
    Configuration menu
    Copy the full SHA
    02eadc1 View commit details
    Browse the repository at this point in the history
  2. Fix part of oppia#5344: Add classroom label to stories on view all sc…

    …reen (oppia#5502)
    
    <!-- READ ME FIRST: Please fill in the explanation section below and
    check off every point from the Essential Checklist! -->
    ## Explanation
    <!--
    - Explain what your PR does. If this PR fixes an existing bug, please
    include
    - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
    issue
      - when this PR is merged.
      -->
    
    Fixes part of oppia#5344
    
    This PR adds classroom label to the promoted stories on view all screen.
    
    ## Screenshots
    |Light Mode|Dark Mode|
    |--|--|
    
    |![image](https://github.com/user-attachments/assets/c97cbdf7-4e2a-43bf-a3f4-d57c0c53811f)|![image](https://github.com/user-attachments/assets/caff0087-cf33-4de3-ad74-44fa375f4f7c)|
    
    ## Essential Checklist
    <!-- Please tick the relevant boxes by putting an "x" in them. -->
    - [x] The PR title and explanation each start with "Fix #bugnum: " (If
    this PR fixes part of an issue, prefix the title with "Fix part of
    #bugnum: ...".)
    - [x] Any changes to
    [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
    files have their rationale included in the PR explanation.
    - [x] The PR follows the [style
    guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
    - [x] The PR does not contain any unnecessary code changes from Android
    Studio
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
    - [x] The PR is made from a branch that's **not** called "develop" and
    is up-to-date with "develop".
    - [x] The PR is **assigned** to the appropriate reviewers
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).
    
    ## For UI-specific PRs only
    <!-- Delete these section if this PR does not include UI-related
    changes. -->
    If your PR includes UI-related changes, then:
    - Add screenshots for portrait/landscape for both a tablet & phone of
    the before & after UI changes
    - For the screenshots above, include both English and pseudo-localized
    (RTL) screenshots (see [RTL
    guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
    - Add a video showing the full UX flow with a screen reader enabled (see
    [accessibility
    guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
    - For PRs introducing new UI elements or color changes, both light and
    dark mode screenshots must be included
    - Add a screenshot demonstrating that you ran affected Espresso tests
    locally & that they're passing
    theMr17 authored and adhiamboperes committed Aug 24, 2024
    Configuration menu
    Copy the full SHA
    496ca4c View commit details
    Browse the repository at this point in the history
  3. Fix part of oppia#5344: Enable Multiple Classrooms (oppia#5510)

    <!-- READ ME FIRST: Please fill in the explanation section below and
    check off every point from the Essential Checklist! -->
    ## Explanation
    <!--
    - Explain what your PR does. If this PR fixes an existing bug, please
    include
    - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
    issue
      - when this PR is merged.
      -->
    
    Fixes part of oppia#5344
    
    Toggles the Multiple Classrooms Feature Flag on.
    
    ## Essential Checklist
    <!-- Please tick the relevant boxes by putting an "x" in them. -->
    - [x] The PR title and explanation each start with "Fix #bugnum: " (If
    this PR fixes part of an issue, prefix the title with "Fix part of
    #bugnum: ...".)
    - [x] Any changes to
    [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
    files have their rationale included in the PR explanation.
    - [x] The PR follows the [style
    guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
    - [x] The PR does not contain any unnecessary code changes from Android
    Studio
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
    - [x] The PR is made from a branch that's **not** called "develop" and
    is up-to-date with "develop".
    - [x] The PR is **assigned** to the appropriate reviewers
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).
    
    ## For UI-specific PRs only
    <!-- Delete these section if this PR does not include UI-related
    changes. -->
    If your PR includes UI-related changes, then:
    - Add screenshots for portrait/landscape for both a tablet & phone of
    the before & after UI changes
    - For the screenshots above, include both English and pseudo-localized
    (RTL) screenshots (see [RTL
    guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
    - Add a video showing the full UX flow with a screen reader enabled (see
    [accessibility
    guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
    - For PRs introducing new UI elements or color changes, both light and
    dark mode screenshots must be included
    - Add a screenshot demonstrating that you ran affected Espresso tests
    locally & that they're passing
    theMr17 authored and adhiamboperes committed Aug 24, 2024
    Configuration menu
    Copy the full SHA
    01e02ec View commit details
    Browse the repository at this point in the history
  4. Fix oppia#5486 & part of oppia#5343: Introducing new wiki page for co…

    …de coverage usage and limitations (oppia#5483)
    
    ## Explanation
    Fixes part of oppia#5343 
    Fixes oppia#5486 
    
    ### Project
    [PR 2.6 of Project 4.1]
    
    ### Changes Made
    - This PR introduces 2 new wiki pages:
      - Oppia Android Code Coverage
      - Writing tests with Good Behavioural Coverage
    - Fix to the comment upload feature permission issues with PRs created
    from the forked branches.
      - Split the code coverage workflow into 2
        1.  Core code coverage workflow to handle 
            - Collection of changed files 
            - Bucket partitioning
            - Run coverage in matrices
        2.  Comment upload workflow to handle
            - evaluation of reports 
            - generation of md reports 
            - uploading comments
            - Coverage Status Checks
          (as the later required `pull_request_target`)
    - Fix to oppia#5486 
    - The issue should have arisen as the pr got merged with the branch
    being deleted while the publish comment job still running, finding it
    hard to fetch the pr-issue number to publish a comment.
    - Now the Coverage Check Status was made to be dependent on the comment
    uploader ie. the final Coverage check job occurs only after the
    Evaluation and Comment jobs are done, so it will always have the pr
    reference)
    ```
      check_coverage_results:
        name: Check Code Coverage Results
        needs: [ evaluate-code-coverage-reports, comment_coverage_report ]
    ```
    
    #
    
    ### Reasons for splitting the code_coverage workflow 
    
    The single code_coverage workflow was split into 
    1. **code_coverage** (to run coverages)
    2. **coverage_report** (to generate and publish reports)
    
    ### Separating the comment upload job
    
    - The primary reason is the need to have ability to upload comments from
    PRs opened from a fork branch.
    -
    [`pull_request_target`](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target):
    For workflows that are triggered by the pull_request_target event, the
    GITHUB_TOKEN is granted read/write repository permission, even when it
    is triggered from a fork.
    - Workflows triggered by `pull_request_target` events are run in the
    context of the base branch. Since the base branch is considered trusted,
    workflows triggered by these events will always run, regardless of
    approval settings. [[GitHub
    Docs](https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks)]
    
    ### Separating the evaluation / generation of report job
    
    - While initially it was split to help with 'skip files' - no files
    changed conditional check, with the introduction to 'SKIP' status check,
    it doesn't continue to serve the mentioned purpose.
    - But if we still have it as one workflow then the flow will work as
    such:
    
    Workflow 1: **code_coverage** 
      - compute changed files
      - run coverage (needs compute changed files)
      - evaluate / generate md
      - code coverage check result
    
    Workflow 2: **coverage_report**
      - comment publication
      
    If no `.kt` files changes are detected
      - compute changed files
      - skips run coverage 
      - skips evaluate / generate md
      - pass code coverage check result 
    
    As the workflow was concluded as success, the 2nd workflow runs as,
      - failed comment publication (as no report is generation due to skip)
    
    But expectation is to still produce a pass check for the comment
    publication. (either to at least skip or upload a skip status as
    coverage comment report)
    
    - With moving it to a separate workflow allows us to not make the
    evaluation / generation jobs rely on the Run coverage job making it
    independently behave once the code_coverage workflows are completed
    successfully.
    - And it checks if pb files are generated and based on that it decides
    whether to generate PASS, FAIL or SKIP status checks.
    
    ### Separating the coverage status check result job
    
    There are 2 main reasons to moving it to new workflow. While it would
    still make sense to have it with the 1st workflow itself after Run
    coverage, the drawbacks are,
    
    - If the check coverage status result was left with the 1st workflow,
    then when the Run coverage job completes in the 1st workflow the
    coverage status check result is set to true on success even before the
    sibling part of upload comment is done, making it an incomplete status
    result.
    - oppia#5486 occurred as it lost its reference to the pr number, so making
    the coverage check status result dependent on (needs) the comment upload
    job should resolve this by only allowing the PR to be closed once the
    comment is uploaded.
    
    #
    
    **Todo:**
    - **[Done]** Add a new wiki page for "Writing effective tests / Writing
    test with good behavioural coverage"
    
    
    ## Essential Checklist
    - [x] The PR title and explanation each start with "Fix #bugnum: " (If
    this PR fixes part of an issue, prefix the title with "Fix part of
    #bugnum: ...".)
    - [x] Any changes to
    [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
    files have their rationale included in the PR explanation.
    - [x] The PR follows the [style
    guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
    - [x] The PR does not contain any unnecessary code changes from Android
    Studio
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
    - [x] The PR is made from a branch that's **not** called "develop" and
    is up-to-date with "develop".
    - [x] The PR is **assigned** to the appropriate reviewers
    ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).
    
    ## For UI-specific PRs only
    If your PR includes UI-related changes, then:
    - Add screenshots for portrait/landscape for both a tablet & phone of
    the before & after UI changes
    - For the screenshots above, include both English and pseudo-localized
    (RTL) screenshots (see [RTL
    guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
    - Add a video showing the full UX flow with a screen reader enabled (see
    [accessibility
    guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
    - For PRs introducing new UI elements or color changes, both light and
    dark mode screenshots must be included
    - Add a screenshot demonstrating that you ran affected Espresso tests
    locally & that they're passing
    
    ---------
    
    Co-authored-by: Ben Henning <ben@oppia.org>
    2 people authored and adhiamboperes committed Aug 24, 2024
    Configuration menu
    Copy the full SHA
    1a68c9f View commit details
    Browse the repository at this point in the history
  5. Fix lint issue

    adhiamboperes committed Aug 24, 2024
    Configuration menu
    Copy the full SHA
    2091fc3 View commit details
    Browse the repository at this point in the history

Commits on Aug 25, 2024

  1. Revert test changes

    adhiamboperes committed Aug 25, 2024
    Configuration menu
    Copy the full SHA
    54718ea View commit details
    Browse the repository at this point in the history