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

dashboard: scripts refactored, unit tests enabled #12923

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

ppitonak
Copy link
Contributor

What does this PR do?

Running mvn clean install did not run unit test. This PR

  1. changes npm scripts so that npm install only installs dependencies and doesn't run build
  2. updates pom.xml so that it uses yarn binary correctly
  3. updates the Dockerfile to call npm install just once, not to use npx unnecessarily and run unit tests

KNOWN ISSUE:

  • yarn is installed in Dockerfile but the base image already contains yarn, the installed version is not used
  • possible solutions: don't run npm i yarn in Dockerfile and (optionally) remove it from package.json OR call yarn binary explicitly using whole path (node_modules/.bin/yarn) as it's done in pom.xml

What issues does this PR fix or reference?

Release Notes

N/A

Docs PR

@che-bot
Copy link
Contributor

che-bot commented Mar 19, 2019

Can one of the admins verify this patch?

2 similar comments
@che-bot
Copy link
Contributor

che-bot commented Mar 19, 2019

Can one of the admins verify this patch?

@che-bot
Copy link
Contributor

che-bot commented Mar 19, 2019

Can one of the admins verify this patch?

@qurben
Copy link
Contributor

qurben commented Mar 19, 2019

On the known issue: running npm i yarn creates a package-lock.json file, because there is none yet, this can cause warnings. So this command actually should be removed.

I'd use npx yarn for the native build, so that it can work with just installing node and it seems as fast as just calling yarn

@olexii4
Copy link
Contributor

olexii4 commented Mar 20, 2019

@ppitonak Good improvement. Thank you.
I am checking. I see several more copy/paste typos. One of them broke namespace 'che' and we have errors after.
https://github.com/eclipse/che/blob/master/dashboard/src/components/typings/che.d.ts#L176
I changed to

    export interface ICheMachineSourceTypes {
      TOOL: string;
      RECIPE: string;
    }

And something else. I will check.

@ppitonak
Copy link
Contributor Author

@qurben do you want me to remove that npm i yarn?
@olexii4 yes, there are a lot of warnings but I didn't want them to mix into this PR

@qurben
Copy link
Contributor

qurben commented Mar 20, 2019

@olexii4 In #12826 the errors with che namespace and other ts errors are fixed.

@ppitonak I think that would really make sense (I don't have any authority in this repo though)

dashboard/Dockerfile Outdated Show resolved Hide resolved
@olexii4
Copy link
Contributor

olexii4 commented Mar 22, 2019

@ppitonak I found the problem: you didn't fix the native build 'mvn clean install -Pnative'.

<!-- build user dashboard -->
 <exec dir="${basedir}" executable="node_modules/.bin/yarn" failonerror="true">
       <arg value="build" />
</exec>

It should be added in this line:
https://github.com/eclipse/che/blob/c88e07d78354f29926c5462be51face8cbd83978/dashboard/pom.xml#L225
Or return this back

 "postinstall": "yarn run typings && yarn run build" 

WDYT?

@ppitonak
Copy link
Contributor Author

@olexii4 you are right that it's broken now but your solution is not 100% correct. I pushed new version.

@@ -197,17 +197,17 @@
<artifactId>maven-antrun-plugin</artifactId>
<executions>
<execution>
<id>install-yarn</id>
<id>install-deps</id>
<phase>compile</phase>
<goals>
<goal>run</goal>
</goals>
<configuration>
<target>
<!-- install yarn -->
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not

install yarn 

now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it

@olexii4
Copy link
Contributor

olexii4 commented Mar 22, 2019

So many people, so many minds. Everyone has his own ideas ))

Signed-off-by: Pavol Pitonak <ppitonak@redhat.com>
@ppitonak
Copy link
Contributor Author

@olexii4 I have two guiding principles in this PR

  1. it should be logical so that new team member will understand what it does, e.g. install deps step only installs dependencies, doesn't build the project etc.
  2. the same commands are called whenever you use maven (docker or native), docker manually or yarn directly - this proved to be a correct approach in my past projects

@olexii4
Copy link
Contributor

olexii4 commented Mar 22, 2019

Logically. You are quite right. It happens depends on the long story of this project. A long time ago it was a separate repo...
I agreed with it for the first time. Told you thanks and a note 'good improvement'. And immediately approved it.

I just note you previously what you lost in the simplest way. And you know - I tried to avoid it.

Thank you a lot. Good job.

@ppitonak
Copy link
Contributor Author

When will the Jenkins job start?

@olexii4
Copy link
Contributor

olexii4 commented Mar 22, 2019

If I am not mistaken: 22.00 or 23.00

@ppitonak
Copy link
Contributor Author

@olexii4 I mean the PR check... now I noticed message from @che-bot that an admin needs to approve it

@olexii4
Copy link
Contributor

olexii4 commented Mar 22, 2019

It depends. We do not have a special time. If there will be a free slave...

It can be approved after ci-test

@olexii4
Copy link
Contributor

olexii4 commented Mar 22, 2019

ci-test

@ppitonak
Copy link
Contributor Author

@olexii4 no I cannot run these commands because I'm not part of team and I haven't been white-listed ;)

@ashumilova
Copy link
Contributor

ci-build

@che-bot
Copy link
Contributor

che-bot commented Mar 22, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12923
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@ppitonak
Copy link
Contributor Author

@ashumilova what will happen now? Will somebody review the results of e2e tests?

@ashumilova ashumilova merged commit 9c18159 into eclipse-che:master Mar 25, 2019
@ppitonak ppitonak deleted the dashboard_unit_tests branch March 25, 2019 08:58
@ashumilova
Copy link
Contributor

@ppitonak merged, thank you for cleanups and enabling tests.

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

Successfully merging this pull request may close these issues.

7 participants