È una cattiva pratica modificare il codice rigorosamente a scopo di test

La modifica del codice per renderlo più testabile ha benefici oltre la testabilità. In generale, codice più testabile

• È più facile da mantenere,
• È più facile ragionare,
• È più sciolto e
• Ha un design complessivo migliore, architettonicamente.

Ci sono (apparentemente) forze avversarie in gioco.

• Da un lato, vuoi rafforzare l'incapsulamento
• D'altra parte, vuoi essere in grado di testare il software

I sostenitori del mantenimento di tutti i "dettagli di implementazione" privati sono solitamente motivati dal desiderio di mantenere l'incapsulamento. Tuttavia, mantenere tutto bloccato e non disponibile è un incompreso approccio all'incapsulamento. Se mantenere tutto ciò che non era disponibile era l'obiettivo finale, l'unico codice incapsulato vero sarebbe questo:

static void Main(string[] args)

Il tuo collega propone di rendere questo l'unico punto di accesso nel tuo codice? Tutti gli altri codici dovrebbero essere inaccessibili dai chiamanti esterni?

Quasi. Quindi cosa rende opportuno rendere pubblici alcuni metodi? Non è, in fondo, una decisione progettuale soggettiva?

Non proprio. Ciò che tende a guidare i programmatori, anche a livello inconscio, è, ancora una volta, il concetto di incapsulamento. Ti senti sicuro nell'esporre un metodo pubblico quando protegge correttamente i suoi invarianti .

Non vorrei esporre un metodo privato che non protegge i suoi invarianti, ma spesso puoi modificarlo in modo che non protegga i suoi invarianti, e allora esponilo al pubblico (ovviamente, con TDD, lo fai al contrario).

Aprire un'API per testabilità è una cosa buona , perché quello che sei fare veramente sta applicando il Principio Aperto / Chiuso .

Se hai un solo chiamante della tua API, non sai quanto sia flessibile la tua API. Le probabilità sono, è abbastanza inflessibile. I test fungono da secondo cliente, dandoti un prezioso feedback sulla flessibilità della tua API .

Quindi, se i test suggeriscono di aprire la tua API, fallo; ma mantenere l'incapsulamento, non nascondendo la complessità, ma esponendo la complessità in modo sicuro.

Sembra che tu stia parlando di iniezione di dipendenza . È molto comune, e IMO, abbastanza necessario per la testabilità.

Per affrontare la più ampia domanda se sia una buona idea modificare il codice solo per renderlo testabile, pensalo in questo modo: il codice ha più responsabilità, tra cui a) da eseguire, b) da leggere dagli umani, e c) da testare. Tutti e tre sono importanti, e se il tuo codice non soddisfa tutte e tre le responsabilità, allora direi che non è un codice molto buono. Quindi modifica via!

È un problema di pollo e uova.

Uno dei più grandi motivi per cui è bene avere una buona copertura di test del codice è che ti permette di refactoring senza paura. Ma ti trovi in una situazione in cui devi rifattorizzare il codice per ottenere una buona copertura di prova! E il tuo collega ha paura.

Vedo il punto di vista del tuo collega. Hai un codice che (presumibilmente) funziona, e se vai a rifattorizzarlo, per qualsiasi ragione, c'è il rischio che tu lo infranga.

Ma se si tratta di un codice che dovrebbe avere manutenzione e modifica in corso, si correrà questo rischio ogni volta che si lavorerà su di esso. E rifattorizzare ora e ottenere una copertura di prova ora ti consentirà di assumerti quel rischio, in condizioni controllate, e ottenere il codice in forma migliore per future modifiche.

Quindi direi che, a meno che questo particolare codebase non sia abbastanza statico e non ci si aspetti che abbia un lavoro significativo in futuro, ciò che si vuole fare è una buona pratica tecnica.

Ovviamente, la pratica di business è un intero "nother lattina di worm".

Questa potrebbe essere una differenza nell'enfasi delle altre risposte, ma direi che il codice non dovrebbe essere refactored rigorosamente per migliorare la testabilità. La testabilità è molto importante per la manutenzione, ma la testabilità è non un fine in sé. Pertanto, rinuncio a qualsiasi refactoring fino a quando non è possibile prevedere che questo codice avrà bisogno di manutenzione per portare a termine alcuni aspetti commerciali.

Nel momento in cui determini che questo codice richiederà una certa manutenzione, che sarebbe un buon momento per refactoring per testabilità. A seconda del tuo caso aziendale, può essere una valida supposizione che il codice tutto richieda eventualmente una certa manutenzione, nel qual caso la distinzione che disegno con le altre risposte qui ( ad esempio < a href="https://softwareengineering.stackexchange.com/a/199092/14820"> La risposta di Jason Swett ) scompare.

Per riassumere: la testabilità da sola non è (IMO) un motivo sufficiente per ridefinire un codice base. La testabilità ha un ruolo prezioso nel consentire la manutenzione su una base di codice, ma è un requisito aziendale modificare la funzione del codice che dovrebbe guidare il refactoring. Se non ci sono requisiti aziendali di questo tipo, probabilmente sarebbe meglio lavorare su qualcosa a cui i clienti saranno interessati.

(Il nuovo codice, ovviamente, viene attivamente mantenuto, quindi dovrebbe essere scritto per essere testabile.)

Penso che il tuo collega abbia torto.

Altri hanno menzionato le ragioni per cui questa è già una buona cosa, ma fintanto che ti viene dato il via libera, dovresti stare bene.

La ragione di questo avvertimento è che qualsiasi modifica al codice è dovuta al fatto che il codice deve essere nuovamente sottoposto a test. A seconda di ciò che fai, questo lavoro di test potrebbe effettivamente essere un grande sforzo da solo.

Non è necessariamente il tuo posto in cui prendere la decisione del refactoring e lavorare su nuove funzionalità a beneficio della tua azienda / cliente.

Ho usato strumenti di copertura del codice come parte del test unitario per verificare se tutti i percorsi attraverso il codice sono stati esercitati. Da buon codificatore / tester da solo, di solito coprono l'80-90% dei percorsi di codice.

Quando studio i percorsi scoperti e faccio uno sforzo per alcuni di loro, è allora che scopro bug come casi di errore che "non accadrà mai". Quindi sì, la modifica del codice e il controllo della copertura del test rendono il codice migliore.

Il tuo problema qui è che i tuoi strumenti di test sono schifo. Dovresti essere in grado di prendere in giro quell'oggetto e chiamare il tuo metodo di prova senza cambiarlo - perché mentre questo semplice esempio è davvero semplice e facile da modificare, cosa succede quando hai qualcosa di molto più complicato.

Un sacco di persone hanno modificato il proprio codice per introdurre le classi basate su IoC, DI e interfacce semplicemente per abilitare il test delle unità utilizzando gli strumenti di test di simulazione e unità che richiedono tali modifiche al codice. Non dimagrisco sono una cosa salutare, non quando vedi un codice che è abbastanza semplice e semplice trasformarsi in un incubo di interazioni complesse guidate interamente dalla necessità di rendere ogni metodo di classe totalmente disgiunto da ogni altra cosa . E per aggiungere la beffa al danno, abbiamo molti argomenti sul fatto che i metodi privati debbano essere testati unitamente o no! (ovviamente dovrebbero, qual è il punto di test unitario se testate solo parte del vostro sistema) ma questi argomenti sono più motivati dal punto di vista che è difficile testare questi metodi usando gli strumenti esistenti - immaginate se il vostro strumento di test potrebbe eseguire test su un metodo privato con la stessa facilità di uno pubblico: tutti li testerebbero senza lamentele.

Il problema, ovviamente, è nella natura degli strumenti di prova.

Ora ci sono strumenti migliori disponibili che potrebbero mettere a letto queste modifiche al design per sempre. Microsoft ha Fakes (nee Moles) che ti permette di mozzare oggetti concreti, compresi quelli statici, quindi non è più necessario modificare il codice per adattarlo allo strumento. Nel tuo caso, se hai usato Fakes, sostituiresti la chiamata GetTypes con la tua che ha restituito dati di test validi e non validi, il che è piuttosto importante, il tuo cambiamento suggerito non lo prevede affatto.

Per rispondere: il tuo collega ha ragione, ma forse per le ragioni sbagliate. Non modificare il codice per testare, cambiare lo strumento di test (o l'intera strategia di test per avere più test unitari sullo stile di integrazione invece di test di precisione).

Martin Fowler ha una discussione intorno a quest'area nel suo articolo Mazze non sono stub

Una buona pratica comune è quella di utilizzare log di test e debug delle unità . I test unitari assicurano che se si apportano ulteriori modifiche al programma, la vecchia funzionalità non si interrompe. I log di debug possono aiutarti a tracciare il programma in fase di esecuzione.
A volte capita che anche al di là di questo abbiamo bisogno di avere qualcosa solo per scopi di test. Non è insolito cambiare il codice per questo. Ma la cura deve essere presa in modo tale che il codice di produzione non sia influenzato a causa di ciò. In C ++ e C questo si ottiene usando MACRO , che è un'entità di tempo di compilazione. Quindi il codice di test non appare affatto nell'ambiente di produzione. Non so se tale disposizione ci sia in C #.
Inoltre, quando aggiungi il codice di test nel tuo programma, dovrebbe essere chiaramente visibile che questa porzione di codice viene aggiunta per alcuni scopi di test. Oppure lo sviluppatore che cerca di capire il codice sta semplicemente andando a sudare su quella parte di codice.

Ci sono alcune gravi differenze tra i tuoi esempi. Nel caso di DoSomethingOnAllTypes() , vi è un'implicazione che do something è applicabile ai tipi nell'assieme corrente. Ma DoSomething(Assembly asm) esplicitamente indica che puoi passare qualsiasi ad esso.

Il motivo per cui lo faccio notare è che un sacco di dipendenze-injection-for-testing-only passi fuori dai limiti dell'oggetto originale. So che hai detto " non" rendendo tutto pubblico "", ma questo è uno dei più grandi errori di quel modello, seguito da vicino da questo: l'apertura dei metodi dell'oggetto fino agli usi a cui non sono destinati .

La tua domanda non ha dato molto contesto in cui il tuo collega ha sostenuto, quindi c'è spazio per la speculazione

"cattiva pratica" o non dipende da come e quando vengono apportate le modifiche.

Da parte mia, il tuo esempio per estrarre un metodo DoSomething(types) è ok.

Ma ho visto il codice che è non ok come questo:

public void DoSomethingOnAllTypes()
{
  var types = (!testmode) 
      ? Assembly.GetExecutingAssembly().GetTypes() 
      : getTypesFromTestgenerator();

  foreach (var currentType in types)
  {
     if (!testmode)
     {
        // do something with this type that made the unittest fail and should be skipped.
     }
     // do something with this type (e.g: read it's attributes, process, etc).
  }
}

Queste modifiche hanno reso il codice più difficile da capire perché hai aumentato il numero di possibili percorsi di codice.

Che cosa intendo con come e quando :

se hai un'implementazione funzionante e per il fine di "implementare capacità di test" hai apportato le modifiche, devi testare nuovamente l'applicazione perché potresti aver infranto il tuo metodo DoSomething() .

Il if (!testmode) è più difficile da capire e provare rispetto al metodo estratto.