Compañero de trabajo revisando el código demasiado tarde en el proceso

Durante el verano cambié de trabajo de una gran empresa como ingeniero senior a una empresa mucho más pequeña como ingeniero principal. Ahora superviso a unos 20 ingenieros jr a nivel medio que trabajan en 3 sistemas de reservas de reservas diferentes. Todos los sistemas de reserva se ejecutan en el mismo marco de back-end patentado, administrado por otro equipo que no superviso.

El mes pasado, la noche anterior al lanzamiento de un gran lanzamiento para uno de los sistemas de reserva, un ingeniero sénior ("Clint") del equipo de soporte de back-end dejó un montón de comentarios en una solicitud de incorporación de cambios que habíamos abierto para fusionar nuestro candidato de lanzamiento enMaster

Los comentarios variaron de algo útiles a algo inútiles. Nos fusionamos de Mastertodos modos y al día siguiente le pregunté si podría haber revisado ese código antes en lugar de esperar hasta después de las pruebas de integración. Cuando dejó los comentarios, era el final del día y estábamos preparando un ticket para que nuestro ingeniero de lanzamiento lo implementara a las 4:30 a. m. de la mañana siguiente.

Me dijo que no es su trabajo enseñar a mis ingenieros (tiene razón, no lo es). Pero es difícil para mí entrenar a 20 ingenieros a la vez, incluso con ellos haciendo revisiones por pares y controlando el código de los demás. También me preocupa que mi equipo estuviera un poco desmotivado porque no pudieron hacer nada para abordar los comentarios.

Tenemos otro lanzamiento programado justo después de regresar del Día de Acción de Gracias, y según cómo Clint rechazó todas nuestras invitaciones a reuniones de revisión de código este mes, creo que voy a ver una repetición de lo mismo en un par de días.

No puedo decir si Clint realmente quiere ayudar o simplemente mostrar su ego. Me encantaría que ayudara a entrenar a nuestros desarrolladores junior, pero la forma en que lo está haciendo no ayuda. No creo que mis ingenieros puedan captar todo lo que Clint puede.

¿Cómo puedo decirle a Clint si quiere dar su opinión, tiene que ser en nuestros términos?

EDITAR : Me avergüenza haber omitido este detalle, pero nuestros ingenieros abren solicitudes de incorporación de cambios desde sus ramas de funciones, developmentadonde deben ir estos comentarios (sobre esas solicitudes de incorporación de cambios)... cuando todos esos cambios estén listos para ir a nuestro entorno de producción ( después de que nuestros ingenieros de control de calidad realicen pruebas de integración y verifiquen que los cambios sean seguros de acuerdo con ellos), abrimos un PR para fusionar Masterdespués de que los ingenieros aprueben los cambios y acuerden que no introdujimos nada horrible y luego implementamos a la mañana siguiente

¿Cuál es el papel de Clint en el proceso de relaciones públicas/fusión? ¿Necesita su revisión para poder fusionarse? ¿Él solo está dando consejos?
¿Cuál es el proceso general aquí? Por lo que ha escrito, se han realizado pruebas de integración y tiene un PR abierto para fusionarse con el maestro, y también implica que implementa desde el maestro como resultado. ¿Por qué se realiza el PR después de la prueba de integración? ¿Por cuánto tiempo estuvo abierto el PR? ¿Qué otras oportunidades existían para la retroalimentación? ¿Qué sucede con sus pruebas si se hace un comentario de relaciones públicas que destaca un problema fundamental? Parece que hay mucho que discutir aquí en un contexto más amplio con todo el equipo, y no necesariamente un problema con Clint dejando comentarios en las relaciones públicas en mi humilde opinión.
¿Por qué esperaron hasta el último momento posible para fusionarse? ¿Hay alguna razón por la que no intentaron fusionarse unas horas antes, momento en el que probablemente habría habido suficiente tiempo para abordar cualquier comentario? Si una solicitud de extracción está abierta, se espera que las personas dejen comentarios; eso parece más un problema de su parte que de su parte. Además, ¿cómo se suponía que iba a saber que no debía comentar? Tenga en cuenta que si los comentarios en sí mismos solicitan que básicamente pierda el tiempo cambiando cosas innecesariamente, tiene una pregunta diferente en sus manos.
Es un asunto que OP debe aclarar, pero para todos los comentaristas que sugieren que la RP debería haberse hecho antes, si su proceso es similar a donde trabajo (git flow), entonces la RP/combinar para dominar es cómo es la implementación en producción. motivado. Habría habido muchos PR anteriores para una rama de desarrollo (o troncal) para cada característica individual, y PR para varias ramas de lanzamiento para control de calidad u otros entornos. Las revisiones de código deberían haber ocurrido entonces, no en la fusión final para dominar.
@sleske ¡Gran pregunta! Esa puede ser la fuente de nuestro problema, pero veo que su función es hacernos saber que ninguno de nuestros cambios es dañino según lo que él entiende de la plataforma de reservas. Puedo fusionarme sin su revisión, y él solo está brindando consejos
@Moo Gracias por la atenta respuesta, espero poder describir el proceso general lo suficientemente bien: los ingenieros de control de calidad fusionan las ramas de funciones una por una en nuestra rama de desarrollo, luego implementamos la rama de desarrollo en nuestro entorno de desarrollo para las pruebas de integración. Si las pruebas pasan, fusionan el desarrollo hasta el maestro para las pruebas de humo en un entorno de prueba. Los cambios solo se fusionan con el maestro después de pasar las pruebas de integración y mediante una solicitud de extracción como una oportunidad adicional para detectar cambios importantes. Me gustaría recibir sus comentarios antes, cuando podamos reaccionar correctamente, pero no quiere darlos en ese momento.
@Dukeling Tratamos de mantener la rama principal sincronizada con la producción y la implementamos poco después de fusionarnos con ella. Nuestros ingenieros de control de calidad esperan un día para probar los cambios en la rama de desarrollo antes de fusionarse con Master para realizar pruebas de humo e implementar la producción poco después. Mi pregunta es sobre mantener los comentarios limitados a cosas que bloquearían una implementación cuando la persona que ofrece comentarios no quiere ese tipo de restricción.
@DavidConrad Sí, lo suficientemente cerca. No es automático, pero tratamos de mantener las nuevas confirmaciones fuera de Master hasta que se hayan probado exhaustivamente y estemos listos para implementarlas en producción.
¿Revisión de código su revisión de código? Revisión de revisión de código: "Comentarios interesantes. Equivocados en algunos lugares. Necesita un enfoque más proactivo. Practique una mejor sincronización".
No mencionas nunca informarle a Clint cuándo esperas que complete los comentarios. ¿Has comunicado algún tipo de línea de tiempo con él? Si tiene líneas de tiempo con las que necesita trabajar, él necesita saberlo para comunicar sus tareas y prioridades a sus superiores.

Respuestas (10)

A menos que Clint encuentre errores importantes de "no implementar", simplemente le agradecería sus comentarios y le explicaría cómo pretende abordar los puntos que plantea (si son válidos) en versiones futuras.

Si él tiene un problema con esto, entonces tienes la oportunidad de explicar por qué sería mejor para él dar su opinión antes.

En última instancia, es su problema que esté dando su opinión tan tarde. No lo haga suyo tomando la retroalimentación como una falla personal o del equipo.

+1 esta es la respuesta correcta. Los comentarios son comentarios, pero como dice la respuesta, a menos que "Clint" encuentre algún tipo de problema de bloqueo, los comentarios se abordarán más adelante. Si "Clint" no está contento con eso, puede revisar el código antes.
¿Aún dirías que es su problema si él no supiera que querían fusionarse esa noche?
No es su problema si esta era su primera oportunidad real de dar su opinión. Y aunque lo fuera, su problema es tu problema es el problema de todos porque todos son parte del mismo equipo y todos desean que tenga éxito, ¿no?
No veo cómo es el problema de Clint. Clint es parte de otro equipo, parece que está dando retroalimentación independientemente del proceso. El problema es que OP no lo percibe como muy útil (debido al tiempo). Creo que nadie tiene una idea clara de lo que debe ser la revisión del código, o cada uno tiene su propia idea. Puede servir para muchos propósitos. Deberían estar en la misma página.
Creo que esta respuesta es correcta, simplemente no veo ningún 'problema' aquí. ¿Quizás estoy malinterpretando? Clint proporcionó nueva información, parte de la cual OP cree que es valiosa. Impresionante. Eso siempre es bueno. Si esa información indica que no puede implementar o fusionar o lo que sea, entonces es MEJOR saberlo lo antes posible. Si esa información indica problemas menores, increíble, regístrela y clasifíquela adecuadamente. No hay problemas aquí. Si quieres ser más receptivo a los comentarios de Clint, entonces seguro, necesitas involucrarlo antes, pero todavía veo esto como una situación perfectamente buena tal como está.
Esta es la forma en que una empresa debe manejarlo. Es un error pensar que cada comentario de revisión de código requiere una acción. Una vez que haya detectado los errores obvios (con suerte por parte de la primera persona en revisar), muchos de los comentarios serán "cosas que sería bueno hacer" o "cosas que podría hacer mejor la próxima vez" o simplemente "cosas que podría hacer de manera diferente pero tal vez siga haciendo lo mismo".
Lol, es problema de Clint en el sentido de que SI él quiere que sus comentarios se tomen en serio, ENTONCES depende de Clint proporcionar dichos comentarios lo suficientemente temprano en el proceso.
¿Comentario al final del día para un despliegue a las 4:30 a. m. del día siguiente? Diablos, cambié los procedimientos para permitir implementaciones solo de lunes a miércoles porque alguien metió la pata en una implementación del viernes posterior al día laboral y creó una enorme montaña de trabajo el lunes. Clint no debería haber esperado que los comentarios fueran procesables, excepto por los errores más graves de tipo show-stopper.
Esto no aborda cómo discutir este problema con el equipo. Pero es bastante obvio: le explica al equipo que esta es una retroalimentación muy útil que les permitirá hacerlo mejor la próxima vez. Ningún código implementado es perfecto y siempre hay cosas que puedes hacer mejor y alguien que te lo indica te está ayudando a aprender.
También agregaría que esto también es un problema de gestión. Todas las revisiones de código deben realizarse antes del control de calidad. Las correcciones de código previas al control de calidad son económicas, las correcciones de código posteriores al control de calidad son costosas porque tiene que pasar el control de calidad nuevamente; solo se arregla si es un problema. Si alguien está haciendo un control de calidad posterior a la revisión del código, eso está desperdiciando el dinero de las empresas. O el proceso debe abordarse para que la revisión del código se realice a tiempo, o no se moleste en absoluto.
@RobP. El problema que veo es que OP se está tomando los comentarios de Clint demasiado en serio. Claramente, no tiene la intención de aplicarse al lanzamiento que se está realizando, ya que es poco probable que Clint sea un idiota, por lo que debe tenerse en cuenta para futuros lanzamientos.

Se requiere una revisión de código para la versión o no se requiere. Si se requiere, entonces el revisor debe ser responsable de revisar esto a tiempo. Debe tener el poder de ir a su escritorio y decir "deje todo lo demás y haga esta revisión, o no podremos llegar a la fecha de lanzamiento". Y debe tener el poder de decir "lo siento, no pudimos publicar a tiempo porque la revisión no se hizo a tiempo".

Si no es necesario, entonces liberas.

PD. Si a Clint se le da un día para revisar semanas o meses de trabajo, eso parece indicar que la revisión no era necesaria. Pero si Clint encuentra problemas en este corto tiempo, eso parece indicar que las revisiones anteriores no fueron tan buenas como deberían haber sido.

+1: la responsabilidad sin el poder de hacer realmente las cosas es un problema que he visto más a menudo de lo que me gustaría.
Dado que aparentemente a "Clint" se le dio menos de un día para revisar las semanas-hombre o quizás los meses-hombre de trabajo, esta respuesta parece bastante equivocada. Hacer que el revisor sea "responsable" de revisar el código más rápido de lo que realmente es posible para ellos revisarlo es claramente irrazonable. Y dado que Clint aparentemente logró dejar algunos comentarios útiles en las pocas horas que tuvo, parece aún más perverso sugerir que todo se arreglaría si solo el OP tuviera la capacidad de obligar a Clint a abandonar sus propias prioridades y revisar o a culpar a Clint por ser demasiado lento.
@MarkAmery No veo en la pregunta ninguna indicación de que el OP haya limitado el tiempo de Clint. Si OP tuviera el poder de preguntar "¿cuánto tiempo necesita para la revisión?" y luego poder decir "OK, por lo que debe comenzar en la fecha X o antes, porque necesitamos comentarios antes de Y", entonces estaría bien. Si OP no puede hacerlo, no debe ser responsable por no responder a los comentarios. Debería ser un problema de gestión, no OP. No se trata de culpar a Clint, sino de no exigir que OP y su equipo hagan lo imposible.
+1 Además, el equipo de OP es libre de abordar los comentarios que no sean bloqueadores antes del próximo lanzamiento.
El tío Ben de @Mołot Peter Parker estuvo cerca cuando dijo que "un gran poder conlleva una gran responsabilidad". Ellos no vienen juntos. Son la misma cosa. Si no tengo el poder de controlar cómo se hace algo, no soy responsable de cómo se hace mal. Demasiadas personas tratan de divorciar el poder y la responsabilidad entre sí, pero simplemente no se puede hacer, y los intentos de hacerlo fracasan. Cada vez que alguien trata de hacerme responsable cuando no tengo poder, digo "¿exactamente qué crees que podría haber hecho de manera diferente para evitar ${BAD_THING}?"; la respuesta suele ser "nada".
@MontyHarder Redefinir el término "responsable" para adaptarlo a la discusión actual no es beneficioso. Si te preguntan sobre la revisión y esperan tu respuesta , y respondes (principalmente porque vinculas internamente la respuesta a tu autoimagen profesional), entonces eso es todo: eres tan responsable de la cosa como se pone. Puede ajustarse a esa definición sin tener el poder o el control total sobre todos los aspectos.
@Mołot Mi lectura de la secuencia de eventos fue que hubo menos de 24 horas entre la apertura del PR (y, por lo tanto, disponible para que Clint lo revisara) y su fusión a pesar de sus objeciones. Esa interpretación ahora es confirmada por el OP. Se abrió en algún momento durante un día laboral, Clint logró introducir alguna revisión, y luego el equipo lo ignoró por completo y se fusionó a las 4:30 a. m. del día siguiente, y el OP culpó a Clint por no haber revisado antes el PR que no Todavía no existe . Me parece bastante loco.
@MarkAmery en tales circunstancias es un problema con el horario. Y sigo pensando que OP debería poder preguntar cuánto tiempo necesita Clint y programar en consecuencia o, si no tiene tal poder, no se debe culpar a OP por el poco tiempo que tuvo Clint o se ignoró la revisión del hecho. El único culpable es el que diseñó un horario tan apretado.
@Mołot Claro, puede estar fuera del alcance del OP. (O puede que no. Está asumiendo, creo, que el horario estaba fuera del control del OP, pero nada lo corrobora en la pregunta; el OP administra un equipo grande y establece o al menos influye de manera plausible en su horario). Independientemente, eso no redime una respuesta que culpa de la situación específica y únicamente a la falta de poder del OP sobre Clint cuando el OP reconoce que Clint inmediatamente dejó comentarios útiles el mismo día en que se le dio algo para revisar. Ninguna cantidad de poder permitirá que el OP obligue a Clint a revisar el código antes de que exista.
@MarkAmery De acuerdo con la pregunta, se invitó a Clint a revisar el código, pero no aceptó. No hay evidencia de que OP esté reteniendo el código hasta el final del proceso. Aparentemente, Clint elige por su cuenta revisar solo al final del proceso. La conclusión es que Clint no cree que la información que podría obtener realmente no valga la pena.
@DavidThornley Sí, según la pregunta, Clint ahora ha sido invitado a las reuniones de revisión de código. Incluso si tomamos su rechazo como evidencia de que elige no revisar hasta el final del proceso (en lugar de, por ejemplo, que coincidan con reuniones más importantes, o que su gerente le indique que no dedique tiempo a hacer el trabajo de otro equipo). para ellos), eso no tiene nada que ver con el incidente anterior descrito por el OP. La conclusión no se sigue en absoluto, ya que no tenemos información sobre por qué Clint rechazó las reuniones.

Parece que hay una serie de problemas aquí, pero todos ellos son abordables.

Problemas de proceso:

  • ¿Cuál es el propósito de esta solicitud de extracción? ¿Está destinado a ser revisado, o es simplemente un medio para fusionarse con el maestro de una sucursal probada y con control de calidad? Si es lo primero, debe considerar una programación diferente para el proceso de revisión. Si es lo último, debe fusionarse casi en el instante en que se crea.
  • ¿Qué haces con los comentarios de revisión "tardíos"? Parece que los comentarios de Clint no pasaron por el proceso de revisión completo, a pesar de que llegaron "tarde". (¿Qué define "tarde"?) Incluso si se ha comprometido a fusionarse y ninguno de los comentarios es un impedimento, debería ser posible clasificar sus comentarios como procesables o no, y responder en el PR según corresponda.

Asuntos personales:

  • Estoy recibiendo un poco de una vibra de "no todos somos un solo equipo" aquí. Esto puede deberse a una frustración comprensible de su parte, pero si respeta a Clint, y entiendo que lo hace, es apropiado tratar de solucionarlo. Es posible que esté rechazando sus solicitudes de reunión de revisión ahora porque está ocupado, o puede ser que sienta que usted realmente no quiere su opinión de todos modos.
  • Rellene para mostrar sus verdaderas intenciones. Si cree que algunas de las cosas que dijo fueron problemas reales, presente multas y asegúrese de que esté incluido en ellas.

Dejar el PR abierto para una "revisión real" hasta la noche anterior a la publicación significa que debe tomar todas las revisiones en serio y no fusionarlas y no enviarlas si hay comentarios sin abordar, o debe cambiar la programación de publicación para que haya tiempo suficiente para completar la revisión y programar o cancelar la inserción (p. ej., los RP que no se fusionaron al menos 24 horas antes de la inserción programada no salen).

Podría ser una buena idea invitarlo a tomar un café y conversar sobre el tema, haciéndole saber que sí, usted valora su aporte, pero que el proceso tal como está ahora significa que no recibió su revisión lo suficientemente pronto como para actuar en él esta vez, enfatizando "esta vez". Hágale saber que está pensando en cambiar el proceso y pregúntele si tiene alguna sugerencia.

Pregúntale qué le facilitaría contribuir y asegúrate de que entienda que aprecias que se haya molestado. Si realmente está tratando de ayudar, entonces ninguna acción sobre sus comentarios también lo desmotiva . Parece que no hubo impedimentos, ya que siguió adelante con la fusión, pero también parece que el trabajo, desde su punto de vista, fue un esfuerzo en vano.

Agradézcale por encontrar los problemas que señaló y hágale saber que tiene la intención de abordar las cosas importantes que mencionó. Deberías considerar hacerlo uno a uno con él y públicamente en relaciones públicas cerradas para dejarle claro a él y al equipo que te alegra que esté ayudando. Mostrar gratitud, en privado y en público, por su interés y por ofrecerse como voluntario para ayudar, incluso si ninguno de los comentarios de la revisión fueron procesables desde el punto de vista de su equipo, es lo más cortés que puede hacer y ayuda a generar solidaridad entre los equipos.

Para ser más específico sobre el motivo "ocupado", Clint puede sentir que lo están arrastrando a un equipo y una carga de trabajo que no es su responsabilidad cuando ya tiene suficiente (¡posiblemente demasiado!) de lo que preocuparse. En ese caso, incluirlo en los informes de errores y hacer otros esfuerzos para que se sienta más involucrado/valorado puede en realidad ir en contra de la relación.
Los ingenieros de soporte de IME a menudo están sobrecargados. Sería útil averiguar las razones de Clint para rechazar las invitaciones a revisar el código (puede que simplemente esté demasiado ocupado). Hacerle saber a Clint que sus contribuciones son valiosas, aunque sea tarde, sería una buena manera de acercarlo al equipo de OP. Si Clint tiene la capacidad, pero siente que no está lo suficientemente cerca como para involucrarse antes, es posible que se sienta incómodo al pisar los dedos de los pies de otras personas.
Sí, por eso sales a tomar un café y lo comentas. Esta vez parece haberse ofrecido como voluntario, por lo que registrarse y ver si le gustaría contribuir y cómo le gustaría contribuir parece ser la forma más directa de resolver las cosas, y en un entorno informal, puede desahogarse si siente que no fue valorado.

¿Cómo puedo decirle a Clint si quiere dar su opinión, tiene que ser en nuestros términos?

A menos que Clint trabaje para usted, o exista algún tipo de proceso formal que dicte cuándo no se permiten los comentarios, no puede obligar a Clint a adherirse a "sus términos".

Puedes ignorar sus comentarios. Puede darle las gracias por los comentarios "algo útiles" para animar a más de esos. Puede agregar elementos a su trabajo pendiente técnico para abordar esos comentarios útiles. Puede continuar invitándolo a las reuniones de revisión de código. Puede alentar a su equipo a no desmotivarse por algunos comentarios tardíos.

1) ¿No hay nadie entre usted y 20 ingenieros que pueda ayudarlo a ser su mentor? Esta es la razón por la que 20 personas son demasiadas personas en un equipo, porque no hay forma de que una sola persona pueda administrar y asesorar a 20 personas. Necesita reducir el tamaño de su equipo, o al menos el tamaño de su responsabilidad, porque está demasiado disperso.

2) Con respecto a la liberación del código, ¿está Clint en su equipo? Si Clint está en su equipo, entonces Clint debería participar en la revisión del código, especialmente si es un ingeniero senior. Debe alentar a sus aprendices a enviar revisiones de código a Clint cuando sea posible (no lo sobrecargue, pero anímelos a involucrar a Clint en el proceso). Si Clint no está en su equipo, a menos que los problemas que Clint encuentre sean errores críticos que interrumpen las operaciones, pídale que los registre como informes de errores; no está en su equipo, no es responsable de su producto.

4) En cuanto a las "reuniones de revisión de código" de Clint: en primer lugar, no estoy seguro de qué es una "reunión de revisión de código". Las revisiones de código son operaciones asincrónicas: el desarrollador envía el código, el revisor de código revisa mientras el desarrollador hace otra cosa, finaliza la revisión, el desarrollador aborda los comentarios, enjuaga, repite. No sé qué es una "reunión de revisión de código", suena tonto. Pero además de eso, si Clint está en su equipo, Clint debería asistir a las reuniones del equipo. Si Clint no está en su equipo, Clint no necesita asistir a las reuniones del equipo. Es tan fácil como eso.

"Reuniones de revisión de código": solíamos hacer esto. Una vez a la semana, uno de los desarrolladores principales revisaba varias solicitudes de fusión recientes y señalaba diferentes ejemplos de código particularmente bueno y malo, y específicamente lo que era bueno o malo al respecto. Entonces todos tendrían una gran discusión al respecto. Es una gran manera de destilar el conocimiento de la experiencia en algo que los jóvenes puedan entender y absorber.
Las revisiones de código pueden ser asíncronas. También pueden ser reuniones grupales donde se recorre el código. Resulta que tener un grupo puede ser más productivo ya que diferentes personas atrapan cosas diferentes.

Alguien tiene que definir el "proceso", por ejemplo, la secuencia en la que suceden las cosas.

Como desarrollador, si no soy yo quien define el proceso, haré una revisión del código (si ese es mi trabajo, como espera mi gerente) cuando el proceso me lo indique y/o cuando alguien me lo pida.

No entiendo la parte de la pregunta que dice "rechacé todas nuestras invitaciones a reuniones de revisión de código este mes".

Por cierto, no estoy seguro de qué es una "reunión de revisión de código". Mi idea de una revisión de código es:

  1. alguien lo codifica
  2. Lo reviso (en mi propio tiempo) y tomo notas
  3. Me reúno con el codificador para discutir mi revisión.

Si soy "senior", quizás no esté escuchando las reuniones de revisión de otras personas.

Para ahorrar tiempo (reducir mi esfuerzo) me gusta revisar el código después de haberlo probado. Todavía podría encontrar errores (por ejemplo, si la prueba no está perfectamente completa), pero es más fácil revisar el código que funciona que el código que no lo hace. La revisión del código que no funciona se denomina "desarrollo y depuración" y lleva más tiempo.

¿Puede hacer que las "pruebas de integración" sean más fáciles, quizás más automatizadas? Para que puedas hacer:

  • Desarrollo
  • Pruebas de integración
  • Revisar
  • Corregir comentarios de revisión
  • Prueba de integración de nuevo

Alternativamente, fusiona antes de la revisión del código (si confía en que la prueba de integración fue suficiente sin revisión), realiza la revisión del código en la rama principal y usa los comentarios de la revisión del código como entrada para lo que podría querer mejorar en una iteración posterior (una iteración posterior). "pique").

-1 para "la revisión del código debe realizarse después de la prueba". Algunas pruebas pueden automatizarse, pero muchas pruebas son manuales (esto es particularmente cierto para el código frontend, pero también puede ser cierto para el código backend). Las pruebas manuales pueden llevar mucho tiempo; Las revisiones de código son generalmente cortas.
Por "después de la prueba" me refiero, como mínimo, después de volver a ejecutar las pruebas de humo automatizadas más cualquier cosa que haga para probar que la nueva función funciona según lo especificado. Los diferentes sistemas tienen una cantidad diferente de automatización en sus pruebas (y dije que si sus pruebas fueran más automatizadas, podrían darse el lujo de hacerlo con más frecuencia).
Las pruebas durante el desarrollo son diferentes a las pruebas en preparación para el lanzamiento y, según las pruebas, pueden llevar una cantidad de tiempo significativa, por lo que desea limitar la frecuencia con la que las ejecuta.
@JoeW Sí, y "la noche antes de un gran lanzamiento" suena bastante [demasiado] tarde. Supongo que "los comentarios variaron de algo útiles a algo inútiles" significa que no encontró ningún impedimento, por lo que el OP hizo bien en lanzar a tiempo cuando lo hicieron. Y eso no es del todo malo: el hecho de que una revisión de código no encuentre obstáculos no es necesariamente algo malo. ¿Cuál es su trabajo, sino "enseñar a mis ingenieros"? ¿Es solo mirar (es decir, fumar) los cambios en la rama de integración antes del lanzamiento?
@ChrisW Definitivamente estoy de acuerdo en que el código no probado no debe revisarse (¡Iría un paso más allá y diría que no debería comprometerse en absoluto!). Sin embargo, como dijo Joe, las pruebas que se debe esperar que un desarrollador ejecute contra su código son diferentes de las pruebas que ejecutaría un control de calidad. Lo primero, seguro. Este último, no tanto.
Creo que te perdiste un problema crítico con cualquier prueba antes de revisar el código en sí. Si las pruebas de integración/humo requieren una cantidad significativa de tiempo para ejecutarse, ¿tiene sentido ejecutar las pruebas y luego hacer una revisión por pares que podría encontrar problemas que requieran que las pruebas se ejecuten nuevamente? ¿No tendría más sentido realizar la revisión de antemano para que se puedan encontrar posibles problemas antes de que se realicen las pruebas, de modo que minimice la frecuencia con la que necesita ejecutar esas pruebas?
@JoeW Supongo que tiene razón, es decir, el OP insinuó que sus "pruebas de integración" eran costosas o requerían mucho tiempo, por lo que tal vez el "proceso" debería especificar que la inspección se realice antes de las pruebas de integración. Si es antes de las pruebas de integración, ¿es eso antes o después de la integración (no lo sé)? Tal vez estaba tratando de poner excusas para Clive, es decir, adivina cuáles podrían ser los motivos de Clive. Si quiere revisar tan tarde, entonces tal vez su inspección debería verse como parte de la prueba de integración, es decir, simplemente "ir/no-ir", y descartar todos sus comentarios de estilo semi-útiles...
... o clasificados, por ejemplo, el OP podría agregarlos opcionalmente a las pautas de revisión de código de su equipo, o usarlos para el entrenamiento de revisión de código.
"Revisar el código que no funciona se denomina 'desarrollo y depuración' y lleva más tiempo". Creo que esto es más una compensación y podría depender de la capacidad/experiencia de los desarrolladores. Revisar antes de las pruebas de control de calidad puede ahorrar en general si encuentra problemas importantes que significan en gran medida renovar el trabajo; no tiene mucho sentido probar el código que debe descartarse en gran medida (por ejemplo, descubre que un código que obtiene unos pocos miles de resultados de informes obtiene datos relacionados un registro a la vez). Intento no buscar errores reales, sino centrarme en cosas como el enfoque general y las normas.

Estoy bastante seguro de que este tipo solo te está diciendo que estás haciendo un mal trabajo. Eso es lo que significa "no es mi trabajo enseñar a sus ingenieros". No está interesado en hacer el trabajo por ti, quiere que lo hagas mejor. Es por eso que revisó el código destinado a la producción (código que firmaste), no el código en proceso. En cuanto a qué hacer al respecto, bueno, ese es un tema para otra pregunta.

Sinceramente, creo que esto da en el clavo. La mayoría de las personas que no están en su equipo no quieren trabajo extra de su equipo. Solo quieren que su equipo haga un buen trabajo para no tener que pensar más en ello.
Estoy de acuerdo en que este es un escenario probable. Dado que "Clint" ha trabajado en el código base durante más tiempo, es probable que también lo conozca mejor.

Tener cuidado.

✓ Desarrollador sénior
✓ Encuentra problemas en el código que nadie más encontró
✓ El equipo está desanimado debido a los informes tardíos
✓ Informa algo sobre "no es su trabajo" pero lo revisó de todos modos
✗ se está preparando para repetir el mismo escenario

No parece tan bueno para el equipo, ni tampoco para el desarrollador sénior.

Pero te digo que tengas cuidado. Asegúrese de comprender todos los comentarios en esa revisión de extracción. Y me refiero a entender realmente. "Ese simplemente está mal". no cuenta a menos que realmente se haya esforzado en comprobarlo.

Probablemente no hay nada importante esperando para ser destructivo para ti. Pero podría haber un error de pérdida de datos extremadamente sutil que descubrió que no se puede revelar en las pruebas. No notarás la diferencia hasta que entiendas todos los comentarios.

Anexo: para restaurar la moral del equipo, asegúrese de tener suficiente tiempo para solucionar todos los problemas que el desarrollador sénior encontró en esa solicitud de incorporación de cambios que el resto del equipo está de acuerdo en que deben solucionarse.

El problema aquí es que su proceso de flujo de trabajo es defectuoso. Las revisiones por pares deben realizarse antes de las pruebas de integración por el simple hecho de que esperar hasta después de que se realicen las pruebas puede sembrar el proceso. Si la revisión por pares encuentra problemas que requieren cambios y las pruebas de integración ya se realizaron, se necesitará más tiempo para regresar y rehacer todas las pruebas una vez que se hayan completado los cambios.

Idealmente, el desarrollador debería escribir y realizar pruebas a medida que desarrolla el código y cualquier prueba final debe realizarse después de que el código sea revisado por pares para asegurarse de que funciona como se espera. Una cosa para recordar es que una revisión por pares puede detectar errores en el código antes de que puedan desactivar un sistema durante las pruebas de integración.

"El problema aquí es que su proceso de flujo de trabajo es defectuoso". No entiendo esta perspectiva. Me parece que el equipo está tratando de hacer revisiones por pares antes de tiempo, y el problema es que Clint los está ignorando hasta el último minuto. El proceso de flujo de trabajo parece estar bien, pero Clint parece negarse a participar de manera efectiva.
@DarthFennec ¿Por qué querría realizar pruebas de integración antes de realizar una revisión por pares del código? Para mí, eso parece defectuoso, ya que tendrá que repetir cualquiera de las pruebas que se realizaron si se encuentra algún problema con el código en la revisión.
No estoy en desacuerdo con eso. Estoy diciendo que OP tampoco parece estar en desacuerdo. "Al día siguiente le pregunté si podría haber revisado ese código antes en lugar de esperar hasta después de las pruebas de integración". Parece que OP y su equipo están tratando de hacer la revisión por pares antes de las pruebas de integración, y el problema declarado es que el revisor (Clint) está actuando en contra de ese proceso de flujo de trabajo.
@DarthFennec Parte del problema aquí es la falta de detalles de la pregunta original que nos permite leer narraciones subyacentes muy diferentes en la historia. Su lectura, supongo, es que el equipo quería que Clint revisara pero eludió sus deberes hasta el último minuto. Mi lectura fue que Clint no estaba involucrado en el proyecto y no tenía autoridad sobre él, luego fue emboscado el día de la fecha límite con un PR gigante de meses-hombre de código basura que no había tenido la oportunidad previa de revisar, y luego el OP se molestó por los comentarios "tardíos" y los fusionó sobre las objeciones de Clint.
@MarkAmery "luego fue emboscado el día de la fecha límite", ¿eso no parece contradecir el comportamiento de OP? ¿Por qué diría OP "Pregunté si podría haber revisado ese código antes en lugar de esperar hasta después de la prueba de integración" si Clint fue emboscado con el código después de la prueba de integración? ¿Por qué la respuesta de Clint sería "no es mi trabajo" en lugar de "no lo sabía antes de las pruebas de integración"? No digo que estés equivocado, simplemente no entiendo cómo llegaste a esa conclusión. Sin embargo, estoy de acuerdo en que hubiera sido mejor si OP hubiera sido más claro al respecto.
@DarthFennec Estoy de acuerdo en que la actitud del OP parece irracional dada mi interpretación, pero la edición del OP lo confirma. La secuencia de eventos es: 1. El equipo de OP realiza un montón de desarrollo en el que Clint no tiene ningún papel, 2. Se abre PR de lanzamiento, 3. Se trae a Clint para verificar la cordura y lo hace el mismo día, dejando algunos comentarios negativos, 4. el lanzamiento ocurre a las 4:30 a.m. del día siguiente de todos modos, 5. OP se queja con Clint sobre los comentarios "tardíos". El comentario "no es mi trabajo" parece razonable por parte de alguien que fue más allá de su deber para dejar comentarios extensos para los miembros de...
@DarthFennec... otro equipo de desarrollo y luego lo reprendieron y les dijeron que deberían haber hecho más, y antes, en un proyecto que, para empezar, no era suyo. La respuesta del OP parece un pequeño cambio de culpa sin mucha justificación, lo cual es, al menos, algo humano y comprensible.
@MarkAmery De la edición: "abrimos un PR para fusionarnos con Master después de que los ingenieros aprueben los cambios" Estaba interpretando que esto significaba que Clint era uno de estos ingenieros, y esperó para aprobar los cambios hasta que se hizo el PR to Master , en lugar de hacerlo antes o en paralelo con las pruebas de integración de QA como se suponía que debía hacer. Pero realmente podría ir de cualquier manera. El hecho de que se le preguntó a OP cuál era el papel de Clint varias veces y no respondió directamente en la edición es sospechoso; si está eludiendo la pregunta a propósito, eso le da crédito a la interpretación de cambiar la culpa.
@MarkAmery No hay nada que demuestre que se requirió la entrada de Clint. (De hecho, la implementación se realizó antes de que se pudieran considerar sus comentarios). OP está invitando a Clint a revisar el código y teme que los comentarios de última hora que no pueden ser útiles para ese lanzamiento vuelvan a ocurrir. OP obviamente quiere que Clint ayude a educar a su equipo, y obviamente Clint no quiere hacerlo. Cualquier acusación de culpabilización en realidad debería mostrar que la culpa existe en primer lugar, y yo no veo eso.

Tiene dos tipos diferentes de revisiones (tres si cuenta su reunión en persona). Eso no es intrínsecamente malo, pero sí significa que debe dejar muy claras sus expectativas para cada circunstancia. Crearía una plantilla de solicitud de incorporación de cambios para su revisión final similar a la siguiente. Esto deja muy claro no solo lo que se espera de esta revisión, sino también cómo abordar sus comentarios con más éxito en el futuro.

<Resumen de cambios>

Esta es una revisión previa a la producción. Su propósito es encontrar problemas mayores como:

  • Fusión de errores
  • Incluir accidentalmente una característica incompleta
  • bichos sensacionales

Salvo que surja alguno de esos tipos de problemas, esta solicitud de incorporación de cambios se fusionará en <fecha y hora>.

Las revisiones de diseño individuales, donde analizamos la legibilidad, la mantenibilidad y la arquitectura general, ya se han completado. Si encuentra ese tipo de problemas aquí, abra un problema o realice los cambios en su propia solicitud de extracción para desarrollo en lugar de saturar esta solicitud de extracción. Las revisiones de diseño se realizaron en las siguientes solicitudes de extracción:

  • <Enlace a PR 1>
  • <Enlace a PR 2>
Compañero de trabajo revisando el código demasiado tarde en el proceso
RespuestasAquíPolítica de privacidad1y, 0v