Skip to content

Fix the TODOs in .github/workflows/nodejs.yml #586

Closed

Activity

added a commit that references this issue on May 9, 2021

Fixes: TheAlgorithms#586 - fix failing tests

63334f1
raklaptudirm

raklaptudirm commented on May 22, 2021

@raklaptudirm
Member
# TODO: error: Line 1: Unexpected token >>

We need to add doctests to all files to fix that issue.

cclauss

cclauss commented on May 22, 2021

@cclauss
MemberAuthor

Or only run doctest on the files that have doctests.

raklaptudirm

raklaptudirm commented on May 22, 2021

@raklaptudirm
Member

Yes but I could not find any exclusion mechanism in the docs (like /* doctest exclude */). We can selectively run test is the workflow, but that means it has to be updated while adding any new algorithm. We need a better alternative.

The second TODO has been fixed here #609 .

cclauss

cclauss commented on May 23, 2021

@cclauss
MemberAuthor

npx doctest a/b.js c/d.js e/f/g.js h/*.js

kkanekii

kkanekii commented on Jul 3, 2021

@kkanekii

Pull request please

raklaptudirm

raklaptudirm commented on Jul 3, 2021

@raklaptudirm
Member

@kkanekii If you want to help, you can create a Pull Request.

ghost

ghost commented on Sep 8, 2021

@ghost

Can someone guide me on creating a pull request?

ghost

ghost commented on Sep 8, 2021

@ghost

I have to change this, right?

name: Node CI
on: [push, pull_request]
jobs:
  build:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        node-version: [14.x]
    steps:
    - uses: actions/checkout@v2
    - name: Use Node.js ${{ matrix.node-version }}
      uses: actions/setup-node@v2
      with:
        node-version: ${{ matrix.node-version }}
    - name: npm install, build, and test
      run: |
        npm install doctest standard --save-dev
-       npx doctest **/*.js || true  # TODO: Add all doctests
+      npx doctest a/b.js c/d.js e/f/g.js h/*.js
        npx standard
        npm ci
        npm run build --if-present
        npm test
      env:
        CI: true
cclauss

cclauss commented on Sep 8, 2021

@cclauss
MemberAuthor

Looks like you at least need another space.

What happens if you leave the command as it is but remove the || true?

We would rather run all the tests, not just hard code in the tests the exist today.

defaude

defaude commented on Oct 5, 2021

@defaude
Contributor

This will get resolved once we got rid of doctest altogether. See #742

4 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @defaude@cclauss@raklaptudirm@kkanekii

      Issue actions

        Fix the TODOs in .github/workflows/nodejs.yml · Issue #586 · TheAlgorithms/JavaScript