Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Create a generic chart lens and use it for pull requests #3411

Merged
14 commits merged into from
Aug 24, 2020
Merged

Create a generic chart lens and use it for pull requests #3411

14 commits merged into from
Aug 24, 2020

Conversation

LucianBuzzo
Copy link
Contributor

Change-type: patch
Signed-off-by: Lucian Buzzo [email protected]

Adds customizable charting, provided by https://github.com/plotly/react-chart-editor

charts

Please remember to write tests for your changes. We aim to automatically
deploy master to production, and we can't safely do this without a solid
test suite!

StefKors
StefKors previously approved these changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@StefKors StefKors left a comment

Choose a reason for hiding this comment

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

LGTM

@joshbwlng
Copy link
Collaborator

Looks like a unit test is failing:
https://ci.balena-dev.com/builds/1372989

  Uncaught exception in apps/ui/lens/misc/Chart/tests/Chart.spec.jsx

  node_modules/plotly.js/dist/plotly.js:104808

  ReferenceError: document is not defined

  addRelatedStyleRule (node_modules/plotly.js/dist/plotly.js:104808:17)
  Object.addStyleRule (node_modules/plotly.js/dist/plotly.js:104799:5)
  Object.1.../src/lib (node_modules/plotly.js/dist/plotly.js:72:9)
  o (node_modules/plotly.js/dist/plotly.js:7:631)
  node_modules/plotly.js/dist/plotly.js:7:682
  Object.694.../build/plotcss (node_modules/plotly.js/dist/plotly.js:102900:1)
  o (node_modules/plotly.js/dist/plotly.js:7:631)
  node_modules/plotly.js/dist/plotly.js:7:682
  Object.14.../src/core (node_modules/plotly.js/dist/plotly.js:242:18)
  o (node_modules/plotly.js/dist/plotly.js:7:631)
  node_modules/plotly.js/dist/plotly.js:7:682
  Object.26../aggregate (node_modules/plotly.js/dist/plotly.js:398:14)
  o (node_modules/plotly.js/dist/plotly.js:7:631)
  r (node_modules/plotly.js/dist/plotly.js:7:797)
  node_modules/plotly.js/dist/plotly.js:7:826
  node_modules/plotly.js/dist/plotly.js:7:88
  Object.<anonymous> (node_modules/plotly.js/dist/plotly.js:7:322)
  Module._compile (node_modules/pirates/lib/index.js:99:24)
  newLoader (node_modules/pirates/lib/index.js:104:7)
  Object.<anonymous> (apps/ui/lens/misc/Chart/Chart.jsx:11:1)
  Module._compile (node_modules/pirates/lib/index.js:99:24)
  newLoader (node_modules/pirates/lib/index.js:104:7)
  Object.<anonymous> (apps/ui/lens/misc/Chart/tests/Chart.spec.jsx:18:1)
  Module._compile (node_modules/pirates/lib/index.js:99:24)
  newLoader (node_modules/pirates/lib/index.js:104:7)

  ✖ apps/ui/lens/misc/Chart/tests/Chart.spec.jsx exited with a non-zero exit code: 1

@LucianBuzzo
Copy link
Contributor Author

@balena-ci rebase

@LucianBuzzo LucianBuzzo force-pushed the generic-charting branch 5 times, most recently from f1c1db4 to 8454ea3 Compare March 31, 2020 10:03
@LucianBuzzo
Copy link
Contributor Author

It looks like adding plotly causes a OOM error:

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

@LucianBuzzo
Copy link
Contributor Author

@balena-ci rebase

@LucianBuzzo
Copy link
Contributor Author

@balena-ci retest

@grahammcculloch
Copy link
Contributor

@balena-ci rebase

@jviotti
Copy link
Contributor

jviotti commented May 15, 2020

@balena-ci rebase

@jviotti
Copy link
Contributor

jviotti commented May 15, 2020

@LucianBuzzo Looks like we are consistently failing to run Webpack on the UI on this particular PR:

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

<--- Last few GCs --->

[8:0x49b9400]   191029 ms: Scavenge 2035.2 (2050.5) -> 2034.3 (2050.7) MB, 12.8 / 0.0 ms  (average mu = 0.229, current mu = 0.164) allocation failure 
[8:0x49b9400]   193688 ms: Mark-sweep 2035.2 (2050.7) -> 2034.3 (2050.7) MB, 2655.9 / 0.0 ms  (average mu = 0.140, current mu = 0.009) allocation failure scavenge might not succeed


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x13a71b9]
Security context: 0x158e012008d1 <JSObject>
    1: /* anonymous */ [0xc90648d641] [/usr/src/*********/node_modules/terser/dist/bundle.min.js:~1] [pc=0x167e21e8ce5e](this=0x1814164177b9 <Sn map = 0x79d7d54d099>,0x2371601f58a9 <AST_SymbolRef map = 0x3bede9228119>,0x276f1c50ab39 <JSFunction a (sfi = 0x21e1a94dc951)>)
    2: _walk [0x276f1c5227e1] [/usr/src/*********/node_modules/terser/dist/bundle.min.js:~1] ...


Writing Node.js report to file: report.20200515.103437.8.0.001.json
Node.js report completed
 1: 0x9ef190 node::Abort() [node]
 2: 0x9f13b2 node::OnFatalError(char const*, char const*) [node]
 3: 0xb5da9e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xb5de19 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xd0a765  [node]
 6: 0xd0adf6 v8::internal::Heap::RecomputeLimits(v8::internal::GarbageCollector) [node]
 7: 0xd1760a v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [node]
 8: 0xd18515 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 9: 0xd1afcc v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
10: 0xce19bb v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node]
11: 0x10246ce v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long*, v8::internal::Isolate*) [node]
12: 0x13a71b9  [node]
Aborted (core dumped)

There is probably some addition that's causing this.

@LucianBuzzo
Copy link
Contributor Author

@jviotti this is probably the addition of the plotly library. I will experiment with chunking the dependencies. In the meantime, I'll change this back to a draft PR

@LucianBuzzo LucianBuzzo marked this pull request as draft May 15, 2020 10:59
@LucianBuzzo LucianBuzzo force-pushed the generic-charting branch 4 times, most recently from 895dbdd to 88efe14 Compare July 24, 2020 14:31
@mbalamat mbalamat added the versionbot/pr-draft Draft PR - Don't merge this PR automatically label Aug 10, 2020
@ghost ghost dismissed StefKors’s stale review August 10, 2020 17:16

Approval reviews not allowed in Draft PRs

@ghost ghost dismissed grahammcculloch’s stale review August 23, 2020 11:28

Approval reviews not allowed in Draft PRs

@mbalamat
test/e2e/ui/pull-requests: Insert test card using context.sdk instead…
ed9558c 
… of browser

Change-type: patch
Signed-off-by: Marios Balamatsias <[email protected]>
@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Users should be able to mark all messages as read from their inbox
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests
Screenshot: Download

@mbalamat
test/e2e/ui/pull-requests: Add e2e test to just go to chart lens
b238aff 
Change-type: patch
Signed-off-by: Marios Balamatsias <[email protected]>
@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests and show the newly added datapoint
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests and show the newly added datapoint
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests and show the newly added datapoint
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests and show the newly added datapoint
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Should be able to navigate to chart lens of pull requests and show the newly added datapoint
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Users should be able to mark all messages as read from their inbox
Screenshot: Download

@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Users should be able to mark all messages as read from their inbox
Screenshot: Download

@mbalamat
test/e2e/ui/pull-requests: Skip test that shows the newly added datap…
2f89b6f 
…oint

Change-type: patch
Signed-off-by: Marios Balamatsias <[email protected]>
@jellyfish-test-bot
Copy link

❌ E2E Tests Failed

Test: Users should be able to mark all messages as read from their inbox
Screenshot: Download

@mbalamat mbalamat removed the versionbot/pr-draft Draft PR - Don't merge this PR automatically label Aug 23, 2020
@mbalamat
Copy link
Contributor

@grahammcculloch I've tried a bunch of things over the weekend to figure out why my e2e test fails on CI but passes locally. I've added another e2e test here that navigates to the chart lens without adding a mock pull-request card and skipped my initial e2e test here. I propose to merge this and I'll figure out the issue with the skipped test case in another PR. If it's ok with you please re-approve, because I added the pr-draft label while testing stuff.

@ghost ghost merged commit 4f81dd4 into master Aug 24, 2020
@ghost ghost deleted the generic-charting branch August 24, 2020 00:13
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mbalamat mbalamat mbalamat left review comments

@grahammcculloch grahammcculloch grahammcculloch approved these changes

@StefKors StefKors StefKors left review comments

@karaxuna karaxuna Awaiting requested review from karaxuna

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LucianBuzzo @joshbwlng @grahammcculloch @jviotti @jellyfish-test-bot @mbalamat @karaxuna @StefKors