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

Agregando componente de Editar grupo #4818

Merged
merged 16 commits into from Nov 26, 2020

Conversation

pabo99
Copy link
Collaborator

@pabo99 pabo99 commented Oct 15, 2020

Descripción

Se agrega el componente de GroupEdit.

Part of: #3494

Checklist:

  • El código sigue la guía de estilo de omegaUp.
  • Se corrieron todas las pruebas y pasaron.
  • Si se está agregando funcionalidad nueva, se agregaron pruebas.
  • Si el cambio es grande (> 200 líneas), hay que intentar partirlo en
    varios pull requests. De preferencia uno para los controladores + phpunit
    y luego otro para la interfaz.

@pabo99 pabo99 marked this pull request as draft October 15, 2020 01:31
@codecov
Copy link

codecov bot commented Oct 15, 2020

Codecov Report

Merging #4818 (1f57765) into master (e1e2ac0) will decrease coverage by 0.02%.
The diff coverage is 82.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4818      +/-   ##
============================================
- Coverage     61.88%   61.86%   -0.03%     
  Complexity     6041     6041              
============================================
  Files           378      383       +5     
  Lines         31180    31306     +126     
  Branches       1201     1208       +7     
============================================
+ Hits          19295    19366      +71     
- Misses        11883    11938      +55     
  Partials          2        2              
Flag Coverage Δ Complexity Δ
javascript 40.06% <82.05%> (+0.31%) 0.00 <0.00> (ø)
php 67.80% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
frontend/www/js/omegaup/lang.en.ts 100.00% <ø> (ø) 0.00 <0.00> (ø)
frontend/www/js/omegaup/lang.es.ts 100.00% <ø> (ø) 0.00 <0.00> (ø)
frontend/www/js/omegaup/lang.pseudo.ts 100.00% <ø> (ø) 0.00 <0.00> (ø)
frontend/www/js/omegaup/lang.pt.ts 100.00% <ø> (ø) 0.00 <0.00> (ø)
frontend/www/js/omegaup/components/group/Edit.vue 82.05% <82.05%> (ø) 0.00 <0.00> (?)
...ontend/www/js/omegaup/components/group/Members.vue 57.57% <0.00%> (ø) 0.00% <0.00%> (?%)
...ontend/www/js/omegaup/components/identity/Edit.vue 34.48% <0.00%> (ø) 0.00% <0.00%> (?%)
.../js/omegaup/components/identity/ChangePassword.vue 57.14% <0.00%> (ø) 0.00% <0.00%> (?%)
...end/www/js/omegaup/components/group/Identities.vue 33.33% <0.00%> (ø) 0.00% <0.00%> (?%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1e2ac0...584e802. Read the comment docs.

@pabo99 pabo99 marked this pull request as ready for review October 24, 2020 17:41
@pabo99
Copy link
Collaborator Author

pabo99 commented Oct 24, 2020

Esto ya está listo para revisión

@pabo99
Copy link
Collaborator Author

pabo99 commented Oct 26, 2020

Veo que hay un error en las pruebas de vue, pero no logro reproducirlo en local:

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

<--- Last few GCs --->

[4268:0x3137150]   425611 ms: Mark-sweep 6126.0 (6166.9) -> 6121.7 (6167.2) MB, 9155.2 / 0.1 ms  (average mu = 0.172, current mu = 0.102) allocation failure GC in old space requested
[4268:0x3137150]   434593 ms: Mark-sweep 6122.6 (6167.2) -> 6121.7 (6167.4) MB, 8975.7 / 0.1 ms  (average mu = 0.091, current mu = 0.001) allocation failure GC in old space requested


<--- JS stacktrace --->

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

    0: ExitFrame [pc: 0x1409219]
Security context: 0x2025807008d1 <JSObject>
    1: /* anonymous */(aka /* anonymous */) [0x8f491311269] [/home/runner/work/omegaup/omegaup/node_modules/nyc/node_modules/@babel/core/lib/transformation/file/merge-map.js:~147] [pc=0x2d9d9a34cd33](this=0x162264fc01b9 <null>,0x2533b9b847d9 <Object map = 0x2b12e3c39b49>)
    2: forEach [0x202580716769](this=0x253463a16d61 <JSArray[225804]>,0x08f491311269 <JSFunc...

 1: 0xa17c40 node::Abort() [/usr/local/bin/node]
 2: 0xa1804c node::OnFatalError(char const*, char const*) [/usr/local/bin/node]
 3: 0xb95a7e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node]
 4: 0xb95df9 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node]
 5: 0xd53075  [/usr/local/bin/node]
 6: 0xd53706 v8::internal::Heap::RecomputeLimits(v8::internal::GarbageCollector) [/usr/local/bin/node]
 7: 0xd5ffc5 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [/usr/local/bin/node]
 8: 0xd60e75 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/usr/local/bin/node]
 9: 0xd6392c v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [/usr/local/bin/node]
10: 0xd2a34b v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationType, v8::internal::AllocationOrigin) [/usr/local/bin/node]
11: 0x106ca7c v8::internal::Runtime_AllocateInOldGeneration(int, unsigned long*, v8::internal::Isolate*) [/usr/local/bin/node]
12: 0x1409219  [/usr/local/bin/node]
Aborted (core dumped)
error Command failed with exit code 134.

@pabo99
Copy link
Collaborator Author

pabo99 commented Oct 30, 2020

Que después de 30 minutos dijo que no pasan las pruebas. ¿Será algo que esté mal en la prueba? Seguiré revisando

@pabo99
Copy link
Collaborator Author

pabo99 commented Oct 30, 2020

Otra alternativa que proponen es actualizar la versión de Node.Js a la 12. Ya la actualicé, aunque no veo diferencias, porque en local no estaba fallando y no sé como actualizarlo en el container

@lhchavez
Copy link
Member

Otra alternativa que proponen es actualizar la versión de Node.Js a la 12. Ya la actualicé, aunque no veo diferencias, porque en local no estaba fallando y no sé como actualizarlo en el container

en qué container quieres actualizarlo? en el de desarrollo local se haría en https://github.com/omegaup/omegaup/blob/master/stuff/docker/Dockerfile.dev-php (ahorita está usando la 10.19), pero GitHub no corre eso: usa la versión default de node documentada en https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu1804-README.md#language-and-runtime , que ya es la 12.19.

@pabo99
Copy link
Collaborator Author

pabo99 commented Oct 31, 2020

Oh, entonces si es un memory leak en el código, porque ya estamos usando toda la memoria y sigue marcándome el mismo error.

@lhchavez
Copy link
Member

Oh, entonces si es un memory leak en el código, porque ya estamos usando toda la memoria y sigue marcándome el mismo error.

lo dudo: nyc usa un montón de memoria, pero no leakea.

aquí estamos llegando al límite de mi conocimiento de frontend D: habrá que preguntar por fuera cómo arreglar esto.

@pabo99
Copy link
Collaborator Author

pabo99 commented Nov 11, 2020

Actualicé nyc para ver si eso arreglaba el error: istanbuljs/nyc#1263 (comment)

Pero al parecer, ya teníamos esa versión y pues sigue con los mismos errores

@lhchavez
Copy link
Member

Actualicé nyc para ver si eso arreglaba el error: istanbuljs/nyc#1263 (comment)

Pero al parecer, ya teníamos esa versión y pues sigue con los mismos errores

sí, la solución no está cerca de nyc, porque nyc no está leakeando.

posiblemente la solución sea usar https://jestjs.io/docs/en/webpack en vez de mocha+nyc: jest ya tiene integrado el soporte de nyc sin tener que darle varias pasadas. o quizás ver si hay manera de no usar webpack para las pruebas (ni idea de si sea viable) y usar Jest+Babel directamente: https://jestjs.io/docs/en/getting-started#using-typescript

@pabo99
Copy link
Collaborator Author

pabo99 commented Nov 26, 2020

Actualicé nyc para ver si eso arreglaba el error: istanbuljs/nyc#1263 (comment)
Pero al parecer, ya teníamos esa versión y pues sigue con los mismos errores

sí, la solución no está cerca de nyc, porque nyc no está leakeando.

posiblemente la solución sea usar https://jestjs.io/docs/en/webpack en vez de mocha+nyc: jest ya tiene integrado el soporte de nyc sin tener que darle varias pasadas. o quizás ver si hay manera de no usar webpack para las pruebas (ni idea de si sea viable) y usar Jest+Babel directamente: https://jestjs.io/docs/en/getting-started#using-typescript

Esta solución parece que si fue la óptima. Ya pasaron las pruebas de Javascript =)

Copy link
Member

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

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

hurra! no más errores misteriosos!

frontend/www/js/omegaup/components/group/Edit.vue Outdated Show resolved Hide resolved
frontend/www/js/omegaup/components/group/Edit.vue Outdated Show resolved Hide resolved
frontend/www/js/omegaup/components/group/Edit.vue Outdated Show resolved Hide resolved
Copy link
Member

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

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

argh, algo que se me fue hace rato. pero ya es lo último!

frontend/www/js/omegaup/components/group/Edit.vue Outdated Show resolved Hide resolved
@lhchavez lhchavez merged commit 1248ee2 into omegaup:master Nov 26, 2020
@pabo99 pabo99 deleted the migrate-group-edit-ui-p1 branch November 26, 2020 23:17
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.

None yet

2 participants