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 frontend testing, require node 12 #15315

Merged
merged 5 commits into from
Apr 8, 2021
Merged

Add frontend testing, require node 12 #15315

merged 5 commits into from
Apr 8, 2021

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Apr 6, 2021

  • Add basic frontend unit testing infrastructure using jest in ESM mode
  • Rename 'make test' to 'make test-backend'
  • Introduce 'make test-frontend' and 'make test' that runs both
  • Bump Node.js requirement to v12. v10 will be EOL in less than a month.
  • Convert all build-related JS files to ESM.

I opted to run frontend tests run as part of the compliance pipeline because they complete fast and are not platform-specific like the golang tests.

make test seems to never be triggered in drone, so this rename only affects users.

⚠️ BREAKING ⚠️

The minimum required node version for compiling the frontend has increased to v12.

@silverwind silverwind added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/testing labels Apr 6, 2021
@silverwind silverwind added this to the 1.15.0 milestone Apr 6, 2021
- Add basic frontend unit testing infrastructure using jest in ESM mode
- Rename 'make test' to 'make test-backend'
- Introduce 'make test-frontend' and 'make test' that runs both
- Bump Node.js requirement to v12. v10 will be EOL in less than a month.
- Convert all build-related JS files to ESM.

I opted to run frontend tests run as part of the compliance pipeline because
they complete fast and are not platform-specific like the golang tests.
@silverwind silverwind added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 8, 2021
@silverwind
Copy link
Member Author

Labeling as breaking because of the bump to the minimum Node.js version.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 8, 2021
docs/config.yaml Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

zeripath commented Apr 8, 2021

do we actually need to up the required version of node for this change?

@6543
Copy link
Member

6543 commented Apr 8, 2021

bump of node version in docs is needed too

@silverwind
Copy link
Member Author

silverwind commented Apr 8, 2021

do we actually need to up the required version of node for this change?

Yes, bump is kind of required unless we want to opt to configure jest in CJS (legacy) mode as the ESM mode requires a feature only present in Node 12.16.0 or above. With node 10 going EOL in around 20 days, I think it's was okay to include the version bump here, which allows us to have the whole codebase in ESM.

Starting in April, many node modules will move to ESM, so doing it now saves us from being forced to do it later.

bump of node version in docs is needed too

I think I handled this with the change in docs/config.yaml, or did I miss anything?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 8, 2021
@6543
Copy link
Member

6543 commented Apr 8, 2021

node eof ref: https://nodejs.org/en/about/releases/

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Will probably need to wire this into drone

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 8, 2021
@silverwind
Copy link
Member Author

Drone is already done: https://drone.gitea.io/go-gitea/gitea/38171/1/9

It's executed as part of the compliance pipeline because it's not platform dependant like the go tests are. Maybe we should move it to the actual test pipelines later for consistency.

@6543 6543 merged commit 0d1a5e0 into go-gitea:master Apr 8, 2021
@silverwind silverwind deleted the jest branch April 8, 2021 11:05
@zeripath
Copy link
Contributor

zeripath commented Apr 8, 2021

So unfortunately this has broken dropzone - possibly others.

See https://try.gitea.io/fnetx/everything/issues/1

index.js?v=d613bc151b1bb2009cfe73a98898d3c0:206 jQuery.Deferred exception: i is not a constructor TypeError: i is not a constructor
    at https://try.gitea.io/js/index.js?v=d613bc151b1bb2009cfe73a98898d3c0:148:1585
    at Generator.next (<anonymous>)
    at f (https://try.gitea.io/js/index.js?v=d613bc151b1bb2009cfe73a98898d3c0:148:1153) undefined
a.Deferred.exceptionHook @ index.js?v=d613bc151b1bb2009cfe73a98898d3c0:206
dropzone.js:6 Uncaught Error: No URL provided.
    at new c (dropzone.js:6)
    at dropzone.js:6
    at Function.oe.discover (dropzone.js:6)
    at oe._autoDiscoverFunction (dropzone.js:8)
    at HTMLDocument.z (dropzone.js:8)
index.js?v=d613bc151b1bb2009cfe73a98898d3c0:206 Uncaught TypeError: i is not a constructor
    at index.js?v=d613bc151b1bb2009cfe73a98898d3c0:148
    at Generator.next (<anonymous>)
    at f (index.js?v=d613bc151b1bb2009cfe73a98898d3c0:148)

Fixing the dropzone.js to read:

export default async function createDropzone(el, opts) {
  const [{default: Dropzone}] = await Promise.all([
    import(/* webpackChunkName: "dropzone" */'dropzone'),
    import(/* webpackChunkName: "dropzone" */'dropzone/dist/dropzone.css'),
  ]);

  Dropzone.Dropzone.autoDiscover = false;
  return new Dropzone.Dropzone(el, opts);
}

makes this one stop - but I suspect there are other potential issues.

zeripath added a commit to zeripath/gitea that referenced this pull request Apr 8, 2021
go-gitea#15315 appears to have caused a change in the way Dropzone is imported - and it
now produces a module rather than the constructor.

This PR rather hackily just adds another Dropzone call to the result.

Signed-off-by: Andrew Thornton <art27@cantab.net>
lunny pushed a commit that referenced this pull request Apr 9, 2021
* Fix Dropzone following #15315

#15315 appears to have caused a change in the way Dropzone is imported - and it
now produces a module rather than the constructor.

This PR rather hackily just adds another Dropzone call to the result.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* use destructured export

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: 6543 <6543@obermui.de>
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants